Try to select sticky mail if in list #7792

paw-hub · 2024-10-22T13:43:22Z

Fix the sticky mail selection behavior so that if we are on the list, it will visually show that mail being selected. If not, don't try to go to that list.

Fixes #7585

charlag · 2024-10-24T08:14:28Z

src/mail-app/mail/view/MailViewModel.ts

@@ -340,7 +354,7 @@ export class MailViewModel {
 		this._folder = folder
 		this.listStreamSubscription?.end(true)
 		this.listStreamSubscription = this.listModel!.stateStream.map((state) => this.onListStateChange(state))
-		this.listModel!.loadInitial()
+		this.listModel!.loadInitial().then(noOp)


why is then(noOp) needed?

It isn't, but my cursed IDE setup complains since loadInitial is async.

Maybe .then() would suffice, but we do .then(noOp) way, way more often.

I can remove the change, though.

there's a magical comment that you can ask IDE to insert that suppresses it

charlag · 2024-10-24T08:15:25Z

src/mail-app/mail/view/MailViewModel.ts

+	private async resetOrInitializeList() {
+		if (this.listModel == null) {
+			// If we don't have a list, load the inbox so that it won't be empty on mobile.
+			this.setListId(await this.getFolderForUserInbox())


isn't it a race to set list id after doing async?

We were already doing this, I think?

tutanota/src/mail-app/mail/view/MailViewModel.ts

Lines 159 to 164 in 82f2217

if (!isSameId(this.stickyMailId, fullMailId)) return

// Make sure that we display *something* in the list, otherwise it'll be empty on mobile

// We could try to open the location of the mail, but it might have been moved around soon after

this.setListId(await this.getFolderForUserInbox())

}

ah that's also not good maybe

charlag · 2024-10-24T08:15:54Z

src/mail-app/mail/view/MailViewModel.ts

@@ -230,12 +235,21 @@ export class MailViewModel {
 	}

 	private async loadExplicitMailTarget(listId: Id, mailId: Id, onMissingTargetEmail: () => unknown) {
+		// shouldStop returning true guarantees we won't load anything if the mail is not in the list already, so
+		// this will be synchronous
+		const listMail = await this.listModel?.loadAndSelect(mailId, () => true)


it should stop if we get called with another id

I misread it, you are trying to select it without loading anything. So this will only select the mail if it's already in the list.

charlag · 2024-11-05T16:30:18Z

src/mail-app/mail/view/MailViewModel.ts

@@ -230,12 +235,21 @@ export class MailViewModel {
 	}

 	private async loadExplicitMailTarget(listId: Id, mailId: Id, onMissingTargetEmail: () => unknown) {
+		// shouldStop returning true guarantees we won't load anything if the mail is not in the list already, so
+		// this will be synchronous
+		const listMail = await this.listModel?.loadAndSelect(mailId, () => true)


I misread it, you are trying to select it without loading anything. So this will only select the mail if it's already in the list.

charlag · 2024-11-05T16:30:41Z

src/mail-app/mail/view/MailViewModel.ts

 		const cached = await this.cacheStorage.get(MailTypeRef, listId, mailId)
 		if (cached) {
 			console.log(TAG, "opening cached mail", mailId)
+			await this.resetOrInitializeList([listId, mailId])


I am confused, above we call loadAndSelect() and this function is supposed to deselect?

This will never get called if loadAndSelect() actually did anything, and it won't do anything if the mail is not currently loaded in the list.

Also, if our list does not contain the mail, then we don't want to change lists, but we also do not want to show a different mail being selected to the one that we are viewing.

Let's say we already have a correct folder loaded and then we click on notification. Let's say the email is already in the list. Wouldn't we loadAndSelect() to select it and then resetOrInitializeList() which will deselect it?

This shouldn't happen because we check if the return value of loadAndSelect is null immediately after calling it.

const listMail = await this.listModel?.loadAndSelect(mailId, () => true) if (listMail) { console.log(TAG, "opening mail from list", mailId) return }

loadAndSelect is unfortunately undocumented, but the return value is the element, if something was selected, or null if not.

return foundItem ?? null

In this case, if we have the correct folder loaded and this function does select something, we will return that value. As such, we won't deselect since we check this return value and return early if so (after logging to console).

Ahhhh yes sorry!

Fix the sticky mail selection behavior so that if we are on the list, it will visually show that mail being selected. If not, don't try to go to that list. Fixes #7585

paw-hub force-pushed the 7585-notification-select branch 3 times, most recently from f878653 to 75647a0 Compare October 22, 2024 14:17

wrdhub linked an issue Oct 23, 2024 that may be closed by this pull request

Opening a mail from the notification does not highlight mail in list #7585

Closed

6 tasks

hrb-hub approved these changes Oct 23, 2024

View reviewed changes

wrdhub approved these changes Oct 23, 2024

View reviewed changes

wrdhub force-pushed the dev-mail branch from 0922a96 to 4139894 Compare October 23, 2024 11:59

charlag reviewed Oct 24, 2024

View reviewed changes

charlag force-pushed the dev-mail branch from 777add6 to 18555a2 Compare October 30, 2024 16:20

wrdhub force-pushed the dev-mail branch from d31ba61 to 95ed069 Compare November 5, 2024 08:52

paw-hub force-pushed the 7585-notification-select branch 2 times, most recently from d777f07 to 73e9551 Compare November 5, 2024 14:16

charlag reviewed Nov 5, 2024

View reviewed changes

charlag approved these changes Nov 5, 2024

View reviewed changes

Try to select sticky mail if in list

5be1b32

Fix the sticky mail selection behavior so that if we are on the list, it will visually show that mail being selected. If not, don't try to go to that list. Fixes #7585

paw-hub force-pushed the 7585-notification-select branch from 73e9551 to 5be1b32 Compare November 6, 2024 08:33

paw-hub merged commit 5be1b32 into dev-mail Nov 6, 2024
1 check passed

paw-hub deleted the 7585-notification-select branch November 6, 2024 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to select sticky mail if in list #7792

Try to select sticky mail if in list #7792

paw-hub commented Oct 22, 2024

charlag Oct 24, 2024

paw-hub Nov 5, 2024

paw-hub Nov 5, 2024

charlag Nov 5, 2024

charlag Oct 24, 2024

paw-hub Nov 5, 2024 •

edited

Loading

charlag Nov 5, 2024

charlag Oct 24, 2024

charlag Nov 5, 2024

charlag Nov 5, 2024

charlag Nov 5, 2024

paw-hub Nov 5, 2024

paw-hub Nov 5, 2024

charlag Nov 5, 2024

paw-hub Nov 5, 2024 •

edited

Loading

charlag Nov 5, 2024

	if (!isSameId(this.stickyMailId, fullMailId)) return

	// Make sure that we display something in the list, otherwise it'll be empty on mobile
	// We could try to open the location of the mail, but it might have been moved around soon after
	this.setListId(await this.getFolderForUserInbox())
	}

Try to select sticky mail if in list #7792

Try to select sticky mail if in list #7792

Conversation

paw-hub commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paw-hub Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paw-hub Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paw-hub Nov 5, 2024 •

edited

Loading

paw-hub Nov 5, 2024 •

edited

Loading