fix for the list not being updated after emails were deleted in search bar #2611
fix for the list not being updated after emails were deleted in search bar #2611csfercoci wants to merge 10 commits into
Conversation
… are queued (#2) * feat: optimistically remove threads from list view when deletion/move tasks are queued Co-authored-by: csfercoci <3026315+csfercoci@users.noreply.github.com> * Merge branch 'master' into copilot/fix-message-deletion-ui
|
1 potential issue found:
Resolved (4 issues)
This PR adds optimistic UI removal of threads from list views when move/delete tasks are queued, adds a global Delete key handler, simplifies search perspective thread removal, and handles non-evaluable matchers in change record processing. The optimistic removal logic was refined across commits to scope removal to matching subscriptions, skip undo tasks, and check move direction.
Waiting for CI checks...
|
| return false; | ||
| } | ||
|
|
||
| tasksForRemovingItems(threads, source?: string) { |
There was a problem hiding this comment.
Behavioral change for Gmail users: The previous implementation checked account.preferredRemovalDestination() and would archive (remove inbox label) for Gmail-style accounts. This replacement always moves to trash via TaskFactory.tasksForMovingToTrash. For Gmail users, pressing Delete/Backspace on search results will now trash threads instead of archiving them, which is more destructive and inconsistent with the archive-by-default behavior in other views.
There was a problem hiding this comment.
Hmm I think this comment is correct, there are sort of separate terms - "remove" vs "trash" vs "archive", and in places where a key binding or helper is tied to "remove", the behavior is meant to be configured by preferredRemovalDestination.
I think for most users clicking the "Backspace" key in a gmail search result list should archive messages and not move them to the trash.
| return dataSource.selection.items() as Thread[]; | ||
| } | ||
|
|
||
| return []; |
There was a problem hiding this comment.
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?
| return false; | ||
| } | ||
|
|
||
| tasksForRemovingItems(threads, source?: string) { |
There was a problem hiding this comment.
Hmm I think this comment is correct, there are sort of separate terms - "remove" vs "trash" vs "archive", and in places where a key binding or helper is tied to "remove", the behavior is meant to be configured by preferredRemovalDestination.
I think for most users clicking the "Backspace" key in a gmail search result list should archive messages and not move them to the trash.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is super interesting - If the issue happens specifically with search, it'd be nice to move this logic down into the SearchQuerySubscription subclass, because we use this QuerySubscription class a lot, and for other model types besides threads. I think that the problem may actually be that the SearchQuerySubscription doesn't refresh like others, but I'm not 100% sure? It's been many years :-)
| if (hasNonEvaluableMatchers) { | ||
| if (itemIsInSet) { | ||
| this._set.updateModel(item); | ||
| } | ||
| unknownImpacts += 1; | ||
| continue; |
There was a problem hiding this comment.
This seems like a separate + pretty consequential change, curious what you were seeing here.
In general this is sort of delicate code because it's used for pretty much everything.
-issue was that after email were deleted they were still persisted in that selection , in filter