Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions app/internal_packages/thread-list/lib/thread-list.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -161,15 +161,15 @@ class ThreadList extends React.Component<
task instanceof ChangeStarredTask
? 'unstar'
: task instanceof ChangeFolderTask
? task.folder.name
: task instanceof ChangeLabelsTask
? 'archive'
: 'remove';
? task.folder.name
: task instanceof ChangeLabelsTask
? 'archive'
: 'remove';

return `swipe-${name}`;
};

props.onSwipeRight = function(callback) {
props.onSwipeRight = function (callback) {
const perspective = FocusedPerspectiveStore.current();
const tasks = perspective.tasksForRemovingItems([item], 'Swipe');
if (tasks.length === 0) {
Expand Down Expand Up @@ -237,7 +237,7 @@ class ThreadList extends React.Component<
event.dataTransfer.setData(`mailspring-accounts=${data.accountIds.join(',')}`, '1');
};

_onDragEnd = event => {};
_onDragEnd = event => { };

_onResize = (event?: any) => {
const narrowStyleWidth = DOMUtils.getWorkspaceCssNumberProperty(
Expand Down Expand Up @@ -309,6 +309,7 @@ class ThreadList extends React.Component<
);
Actions.popSheet();
};

}

export default ThreadList;
29 changes: 28 additions & 1 deletion app/internal_packages/thread-list/lib/thread-toolbar-buttons.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,28 @@ export class TrashButton extends React.Component<{ items: Thread[] }> {
class HiddenGenericRemoveButton extends React.Component<{ items: Thread[] }> {
static displayName = 'HiddenGenericRemoveButton';

_itemsForRemove = () => {
if (this.props.items && this.props.items.length > 0) {
return this.props.items;
}

const dataSource = ThreadListStore.dataSource();
if (!dataSource) {
return [];
}

const focused = FocusedContentStore.focused('thread') as Thread;
if (focused) {
return [focused];
}

if (dataSource.selection && dataSource.selection.count() > 0) {
return dataSource.selection.items() as Thread[];
}

return [];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this is an interesting change, it seems like the thinking here is that if you have both a selection and a focused item, the selection should take precedence? It'd be helpful to have a repro case if you had one in mind - I wonder if this makes more sense in the vertical split view?

I think that this might be a good behavior improvement but it seems odd to do it for just the generic remove action and not the core:archive-item key binding (ArchiveButton) or the TrashButton?

};

_onRemoveAndShift = ({ offset }) => {
const dataSource = ThreadListStore.dataSource();
const focusedId = FocusedContentStore.focusedId('thread');
Expand All @@ -117,8 +139,13 @@ class HiddenGenericRemoveButton extends React.Component<{ items: Thread[] }> {
};

_onRemoveFromView = () => {
const items = this._itemsForRemove();
if (items.length === 0) {
return;
}

const current = FocusedPerspectiveStore.current();
const tasks = current.tasksForRemovingItems(this.props.items, 'Keyboard Shortcut');
const tasks = current.tasksForRemovingItems(items, 'Keyboard Shortcut');
Actions.queueTasks(tasks);
Actions.popSheet();
};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
import React from 'react';
import {
localized,
Folder,
ChangeLabelsTask,
ChangeFolderTask,
AccountStore,
CategoryStore,
TaskFactory,
MailboxPerspective,
Actions,
Expand Down Expand Up @@ -82,31 +77,9 @@ class SearchMailboxPerspective extends MailboxPerspective {
}

tasksForRemovingItems(threads, source?: string) {
return TaskFactory.tasksForThreadsByAccountId(threads, (accountThreads, accountId) => {
const account = AccountStore.accountForId(accountId);
if (!account) {
return [];
}
const dest = account.preferredRemovalDestination();
if (!dest) {
return [];
}
if (dest instanceof Folder) {
return new ChangeFolderTask({
threads: accountThreads,
source: 'Dragged out of list',
folder: dest,
});
}
if (dest.role === 'all') {
// if you're searching and archive something, it really just removes the inbox label
return new ChangeLabelsTask({
threads: accountThreads,
source: 'Dragged out of list',
labelsToRemove: [CategoryStore.getInboxCategory(accountId)],
});
}
throw new Error('Unexpected destination returned from preferredRemovalDestination()');
return TaskFactory.tasksForMovingToTrash({
threads,
source: source || 'Keyboard Shortcut',
});
}
}
Expand Down
3 changes: 2 additions & 1 deletion app/keymaps/base.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"core:print-thread": "mod+p",
"core:copy-mailbox-link": "ctrl+l",
"core:focus-item": "enter",
"core:remove-from-view": ["backspace", "del"],
"core:remove-from-view": ["backspace"],
"core:delete-item": ["del", "delete"],
"core:pop-sheet": "escape",
"core:show-keybindings": "?",

Expand Down
132 changes: 129 additions & 3 deletions app/spec/models/query-subscription-pool-spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import QuerySubscriptionPool from '../../src/flux/models/query-subscription-pool';
import DatabaseStore from '../../src/flux/stores/database-store';
import { Label } from '../../src/flux/models/label';
import { Thread } from '../../src/flux/models/thread';
import { Folder } from '../../src/flux/models/folder';
import { ChangeFolderTask } from '../../src/flux/tasks/change-folder-task';
import { ChangeLabelsTask } from '../../src/flux/tasks/change-labels-task';

describe('QuerySubscriptionPool', function QuerySubscriptionPoolSpecs() {
beforeEach(() => {
Expand Down Expand Up @@ -33,11 +37,11 @@ describe('QuerySubscriptionPool', function QuerySubscriptionPoolSpecs() {

describe('unsubscribe', () => {
it('should return an unsubscribe method', () => {
expect(QuerySubscriptionPool.add(this.query, () => {}) instanceof Function).toBe(true);
expect(QuerySubscriptionPool.add(this.query, () => { }) instanceof Function).toBe(true);
});

it('should remove the callback from the subscription', () => {
const cb = () => {};
const cb = () => { };

const unsub = QuerySubscriptionPool.add(this.query, cb);
const subscription = QuerySubscriptionPool._subscriptions[this.queryKey];
Expand All @@ -48,7 +52,7 @@ describe('QuerySubscriptionPool', function QuerySubscriptionPoolSpecs() {
});

it("should wait before removing th subscription to make sure it's not reused", () => {
const unsub = QuerySubscriptionPool.add(this.query, () => {});
const unsub = QuerySubscriptionPool.add(this.query, () => { });
expect(QuerySubscriptionPool._subscriptions[this.queryKey]).toBeDefined();
unsub();
expect(QuerySubscriptionPool._subscriptions[this.queryKey]).toBeDefined();
Expand All @@ -57,4 +61,126 @@ describe('QuerySubscriptionPool', function QuerySubscriptionPoolSpecs() {
});
});
});

describe('_threadIdsForRemovalTask', () => {
const threadSubscriptionForCategory = (categoryId: string) => {
const threadQuery = DatabaseStore.findAll<Thread>(Thread).where([
Thread.attributes.categories.contains(categoryId),
]);
QuerySubscriptionPool.add(threadQuery, () => { });
return QuerySubscriptionPool._subscriptions[threadQuery.sql()];
};

it('should return threadIds for a ChangeFolderTask when moving out of the subscription category', () => {
const threads = [
new Thread({ id: 't1', accountId: 'a1', folders: [new Folder({ id: 'f1' })] }),
];
const subscription = threadSubscriptionForCategory('f1');
const task = new ChangeFolderTask({
threads,
folder: new Folder({ id: 'trash-folder', role: 'trash', accountId: 'a1' }),
previousFolder: new Folder({ id: 'f1', role: 'inbox', accountId: 'a1' }),
});
const result = QuerySubscriptionPool._threadIdsForRemovalTask(task, subscription);
expect(result).toEqual(['t1']);
});

it('should return null for a ChangeFolderTask when moving into the subscription category', () => {
const threads = [
new Thread({ id: 't1', accountId: 'a1', folders: [new Folder({ id: 'spam-folder' })] }),
];
const subscription = threadSubscriptionForCategory('inbox-folder');
const task = new ChangeFolderTask({
threads,
folder: new Folder({ id: 'inbox-folder', role: 'inbox', accountId: 'a1' }),
previousFolder: new Folder({ id: 'spam-folder', role: 'spam', accountId: 'a1' }),
});
const result = QuerySubscriptionPool._threadIdsForRemovalTask(task, subscription);
expect(result).toBeNull();
});

it('should return null for an undo ChangeFolderTask', () => {
const threads = [
new Thread({ id: 't1', accountId: 'a1', folders: [new Folder({ id: 'f1' })] }),
];
const subscription = threadSubscriptionForCategory('inbox-folder');
const task = new ChangeFolderTask({
threads,
folder: new Folder({ id: 'inbox-folder', role: 'inbox', accountId: 'a1' }),
previousFolder: new Folder({ id: 'trash-folder', role: 'trash', accountId: 'a1' }),
});
task.isUndo = true;

const result = QuerySubscriptionPool._threadIdsForRemovalTask(task, subscription);
expect(result).toBeNull();
});

it('should return threadIds for a ChangeLabelsTask with only removals', () => {
const subscription = threadSubscriptionForCategory('inbox');
const task = new ChangeLabelsTask({
threads: [new Thread({ id: 't2', accountId: 'a1' })],
labelsToRemove: [new Label({ id: 'inbox', role: 'inbox', accountId: 'a1' })],
labelsToAdd: [],
});
const result = QuerySubscriptionPool._threadIdsForRemovalTask(task, subscription);
expect(result).toEqual(['t2']);
});

it('should return null for a ChangeLabelsTask with additions', () => {
const subscription = threadSubscriptionForCategory('inbox');
const task = new ChangeLabelsTask({
threads: [new Thread({ id: 't3', accountId: 'a1' })],
labelsToRemove: [new Label({ id: 'inbox', role: 'inbox', accountId: 'a1' })],
labelsToAdd: [new Label({ id: 'archive', role: 'all', accountId: 'a1' })],
});
const result = QuerySubscriptionPool._threadIdsForRemovalTask(task, subscription);
expect(result).toBeNull();
});
});

describe('_optimisticallyRemoveThreads', () => {
it('should call optimisticallyRemoveItemsById only on matching Thread subscriptions', () => {
const inboxQuery = DatabaseStore.findAll<Thread>(Thread).where([
Thread.attributes.categories.contains('inbox-folder'),
]);
const searchQuery = DatabaseStore.findAll<Thread>(Thread);

QuerySubscriptionPool.add(inboxQuery, jasmine.createSpy('inboxCb'));
QuerySubscriptionPool.add(searchQuery, jasmine.createSpy('searchCb'));

const inboxSubscription = QuerySubscriptionPool._subscriptions[inboxQuery.sql()];
const searchSubscription = QuerySubscriptionPool._subscriptions[searchQuery.sql()];

spyOn(inboxSubscription, 'optimisticallyRemoveItemsById');
spyOn(searchSubscription, 'optimisticallyRemoveItemsById');

const task = new ChangeFolderTask({
threads: [new Thread({ id: 't1', accountId: 'a1' })],
folder: new Folder({ id: 'trash-folder', role: 'trash', accountId: 'a1' }),
previousFolder: new Folder({ id: 'inbox-folder', role: 'inbox', accountId: 'a1' }),
});

QuerySubscriptionPool._optimisticallyRemoveThreads(task);
expect(inboxSubscription.optimisticallyRemoveItemsById).toHaveBeenCalledWith(['t1']);
expect(searchSubscription.optimisticallyRemoveItemsById).not.toHaveBeenCalled();
});

it('should not call optimisticallyRemoveItemsById on non-Thread subscriptions', () => {
const labelQuery = DatabaseStore.findAll<Label>(Label);
const labelKey = labelQuery.sql();
const callback = jasmine.createSpy('callback');
QuerySubscriptionPool.add(labelQuery, callback);
const subscription = QuerySubscriptionPool._subscriptions[labelKey];
spyOn(subscription, 'optimisticallyRemoveItemsById');

const task = new ChangeFolderTask({
threads: [new Thread({ id: 't1', accountId: 'a1' })],
folder: new Folder({ id: 'trash-folder', role: 'trash', accountId: 'a1' }),
previousFolder: new Folder({ id: 'inbox-folder', role: 'inbox', accountId: 'a1' }),
});

QuerySubscriptionPool._optimisticallyRemoveThreads(task);
expect(subscription.optimisticallyRemoveItemsById).not.toHaveBeenCalled();
});
});
});
45 changes: 42 additions & 3 deletions app/spec/models/query-subscription-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ describe('QuerySubscription', function QuerySubscriptionSpecs() {
describe('when initialModels are provided', () =>
it('should apply the models and trigger', () => {
const query = DatabaseStore.findAll<Thread>(Thread);
const threads = [1, 2, 3, 4, 5].map(i => new Thread({ id: i }));
const threads = [1, 2, 3, 4, 5].map((i) => new Thread({ id: i }));
const subscription = new QuerySubscription(query, { initialModels: threads });
expect(subscription._set).not.toBe(null);
}));
Expand Down Expand Up @@ -222,8 +222,8 @@ describe('QuerySubscription', function QuerySubscriptionSpecs() {
jasmine.unspy(Utils, 'generateTempId');

describe('scenarios', () =>
scenarios.forEach(scenario => {
scenario.tests.forEach(test => {
scenarios.forEach((scenario) => {
scenario.tests.forEach((test) => {
it(`with ${scenario.name}, should correctly apply ${test.name}`, () => {
const subscription = new QuerySubscription(scenario.query);
subscription._set = new MutableQueryResultSet();
Expand Down Expand Up @@ -348,4 +348,43 @@ describe('QuerySubscription', function QuerySubscriptionSpecs() {
});
});
});

describe('optimisticallyRemoveItemsById', () => {
it('should remove items from the set and trigger callbacks', () => {
const query = DatabaseStore.findAll<Thread>(Thread);
const threads = [1, 2, 3, 4, 5].map((i) => new Thread({ id: `${i}` }));
const subscription = new QuerySubscription(query, { initialModels: threads });

spyOn(subscription, '_createResultAndTrigger');
subscription.optimisticallyRemoveItemsById(['2', '4']);

expect(subscription._set.offsetOfId('2')).toBe(-1);
expect(subscription._set.offsetOfId('4')).toBe(-1);
expect(subscription._set.ids().length).toBe(3);
expect(subscription._createResultAndTrigger).toHaveBeenCalled();
});

it('should not trigger if no items were in the set', () => {
const query = DatabaseStore.findAll<Thread>(Thread);
const threads = [1, 2, 3].map((i) => new Thread({ id: `${i}` }));
const subscription = new QuerySubscription(query, { initialModels: threads });

spyOn(subscription, '_createResultAndTrigger');
subscription.optimisticallyRemoveItemsById(['99', '100']);

expect(subscription._set.ids().length).toBe(3);
expect(subscription._createResultAndTrigger).not.toHaveBeenCalled();
});

it('should do nothing if _set is null', () => {
spyOn(QuerySubscription.prototype, 'update').andReturn();
const query = DatabaseStore.findAll<Thread>(Thread);
const subscription = new QuerySubscription(query);
subscription._set = null;

expect(() => {
subscription.optimisticallyRemoveItemsById(['1', '2']);
}).not.toThrow();
});
});
});
Loading