Only show each thread once per message list #4970

kesselb · 2021-04-20T15:51:05Z

Todo

Endpoint to delete / move a thread
Delete in envelope list should delete all messages (excl. trash)
Move in envelope list should move all messages (excl. trash)
Delete the latest message in thread view should add the second latest message to the envelope list¹
Move the latest message in thread view should add the second latest message to the envelope list¹
Select multiple threads/messages in envelope to delete/move must still work
Keyboard shortcuts
Rename delete to delete thread in envelope list
Rename move to move thread in envelope list

¹ Is done by the existing sync backend request

lib/Db/MessageMapper.php

ChristophWurst · 2021-04-21T11:50:32Z

Delete the latest message in thread view should add the second latest message to the envelope list

As discussed last week those operations might not actually need special handling. The existing sync logic should be able to see that a new message is "added" to the list.

What we might have to change is the sync logic for new messages. Right now we consider all messages with a higher UID than highest known UID as new. What we can do instead is consider all messages as new whose UID is higher than the lowest known UID and not in the list of known UIDs already. Then you can also detect "new" messages that pop up in the middle of the previous message list.

lib/Db/MessageMapper.php

kesselb · 2021-05-04T15:20:13Z

Peek.2021-05-04.16-27.mp4

Case 1: Delete the latest message in a thread. The message is removed from the envelope list but none of the other messages show up.

I'm not sure yet if changing the findNewIds method is possible. Right now we take the known id's, find the max uid (on master) / max sent_at (this branch) and query for all messages > max uid/sent_at.

As suggest we may change max to min uid/sent_at of the known id's. This will not work for my example case as the known id's are a few. However if a user has more than 20 messages it should work.

In addition we could lookup the envelopes store on message delete if there is another message of the same thread and add it to visible envelopes list.

kesselb · 2021-05-19T13:17:07Z

View
Inbox (and other mailboxes but not trash): Show messages in the thread view but hide messages in trash.
Trash: Show messages in thread view that are in the trash. Hide messages from other mailboxes.
Hiding messages is done on client.

Delete
Inbox (and other mailboxes but not trash): Delete a thread should move all messages (in inbox and other mailboxes but not trash) to trash mailbox.
Trash: Delete a thread should delete the deleted messages of a thread that are in trash.

Move
Inbox (and other mailboxes but not trash): Move a thread should move all messages (in inbox and other mailboxes but not trash) to target mailbox.
Trash: Move a thread should move the deleted messages of a thread that are in trash.

kesselb · 2021-05-19T19:44:27Z

src/store/actions.js

@@ -770,4 +771,16 @@ export default {
 			tagId: tag.id,
 		})
 	},
+	async deleteThread({ getters, commit }, { envelope }) {
+		commit('removeEnvelope', { id: envelope.databaseId })


The envelope to remove is the envelope in the list (e.g. the newest/latest message in a thread). If the other envelopes of a thread are loaded we still have references in other vuex stores (e.g. mailbox.envelopeLists). removeEnvelope mutation has a check if the envelope to remove is a "mounted" envelope and does not remove references for "unmounted" envelopes.

src/store/getters.js

tests/Integration/Db/ThreadMapperTest.php

kesselb · 2021-05-26T12:19:38Z

src/components/EnvelopeList.vue

-		deleteAllSelected() {
-			this.selectedEnvelopes.forEach(async(envelope) => {
+		async deleteAllSelected() {
+			for (const envelope of this.selectedEnvelopes) {


The change forEach to for...of is not necessary for the threading improvements. forEach is not blocking. If a user selects 10 threads / messages we emit 10 xhr request. for...of will process the selected envelopse sequentially. Ideally would be a "loading" state to signal that mail is working on something.

kesselb · 2021-05-26T12:23:57Z

src/components/MoveModal.vue

-					id,
-					destMailboxId: this.destMailboxId,
-				})))
+				for (const envelope of envelopes) {


The change Promise.all to for...of is not necessary for the threading improvements. Promise.all will dispatch a moveMessage / moveThread action for each envelope passed to the MoveModal. Ideally would be a "loading" state to signal that mail is working on something.

kesselb · 2021-05-27T19:12:01Z

lib/Db/MessageMapper.php

 			);

-		$qb->select('id')
-			->from($this->getTableName())
+		$select


Chunking in statements for Oracle Compliance #4947 introduced chunking for in expressions. I need to check the findNewIds query to make sure it still works as expected.

st3iny

I just tested this PR on my local instance and here are some things I noticed:

Deleting the most recent message from a thread in the thread envelope view will make the thread disappear from the list. Triggering a sync will make the thread reappear.
A thread is still shown after moving it to another folder, switching to a third folder and then back again. It will be shown in both mailboxes a the same time. Moving the thread back to the original folder will show it twice in the list. A page reload will fix the duplication.
Marking a thread important using the context menu of an envelope in the list will not change the label of the button (Mark important -> Mark unimportant).

I only looked at the code briefly.

PS: I tested f97fc51. The most recent commit on this branch will trigger an internal server error upon visiting the mail app.

EDIT: Removed an unreproducible issue.

lib/Db/ThreadMapper.php

ChristophWurst reviewed Apr 21, 2021

View reviewed changes

lib/Db/MessageMapper.php Outdated Show resolved Hide resolved

ChristophWurst changed the title ~~Enh/3652/only show each thread once per message list~~ Only show each thread once per message list Apr 21, 2021

kesselb commented Apr 21, 2021

View reviewed changes

lib/Db/MessageMapper.php Outdated Show resolved Hide resolved

kesselb force-pushed the enh/3652/only_show_each_thread_once_per_message_list branch from fac22ca to 96cc26b Compare May 4, 2021 09:41

kesselb force-pushed the enh/3652/only_show_each_thread_once_per_message_list branch 2 times, most recently from cd141ea to d1682e8 Compare May 18, 2021 09:45

kesselb force-pushed the enh/3652/only_show_each_thread_once_per_message_list branch from 86f6d4b to 53a63d6 Compare May 19, 2021 12:36

kesselb commented May 19, 2021

View reviewed changes

src/store/getters.js Outdated Show resolved Hide resolved

kesselb force-pushed the enh/3652/only_show_each_thread_once_per_message_list branch 5 times, most recently from 0667e6b to 9a911bf Compare May 25, 2021 21:11

kesselb commented May 25, 2021

View reviewed changes

tests/Integration/Db/ThreadMapperTest.php Outdated Show resolved Hide resolved

kesselb force-pushed the enh/3652/only_show_each_thread_once_per_message_list branch from 01c1319 to 7a2823b Compare May 26, 2021 09:13

kesselb commented May 26, 2021

View reviewed changes

kesselb force-pushed the enh/3652/only_show_each_thread_once_per_message_list branch from 874c7ac to f97fc51 Compare May 26, 2021 16:48

kesselb marked this pull request as ready for review May 26, 2021 16:53

kesselb added this to the v1.10.0 milestone May 26, 2021

kesselb added 3. to review enhancement labels May 26, 2021

kesselb commented May 27, 2021

View reviewed changes

ChristophWurst requested a review from st3iny June 1, 2021 07:23

st3iny reviewed Jun 1, 2021

View reviewed changes

lib/Db/ThreadMapper.php Outdated Show resolved Hide resolved

kesselb closed this Jun 1, 2021

kesselb force-pushed the enh/3652/only_show_each_thread_once_per_message_list branch from e812d50 to fbec019 Compare June 1, 2021 18:36

kesselb mentioned this pull request Jun 2, 2021

Only show each thread once per message list #5106

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only show each thread once per message list #4970

Only show each thread once per message list #4970

kesselb commented Apr 20, 2021 •

edited

ChristophWurst commented Apr 21, 2021

kesselb commented May 4, 2021 •

edited

kesselb commented May 19, 2021

kesselb May 19, 2021 •

edited

kesselb May 26, 2021 •

edited

kesselb May 26, 2021

kesselb May 27, 2021 •

edited

st3iny left a comment •

edited

Only show each thread once per message list #4970

Only show each thread once per message list #4970

Conversation

kesselb commented Apr 20, 2021 • edited

ChristophWurst commented Apr 21, 2021

kesselb commented May 4, 2021 • edited

kesselb commented May 19, 2021

kesselb May 19, 2021 • edited

Choose a reason for hiding this comment

kesselb May 26, 2021 • edited

Choose a reason for hiding this comment

kesselb May 26, 2021

Choose a reason for hiding this comment

kesselb May 27, 2021 • edited

Choose a reason for hiding this comment

st3iny left a comment • edited

Choose a reason for hiding this comment

kesselb commented Apr 20, 2021 •

edited

kesselb commented May 4, 2021 •

edited

kesselb May 19, 2021 •

edited

kesselb May 26, 2021 •

edited

kesselb May 27, 2021 •

edited

st3iny left a comment •

edited