Use custom ListModel for getting mails #8308
Conversation
This removes MailViewModel having to keep track of MailSetEntry<->Mail, and it removes a lot of legacy code for getting Mail vs MailSetEntry, as all of the implementation details for getting mails are now in MailListModel. Additionally, everyone uses mailsets, so the legacy code is no longer necessary. Closes #8247 Co-authored-by: hrb-hub <[email protected]>
paw-hub
force-pushed
the
8247-refactor-non-conversation-list
branch
from
c502aad to
3356e4b
Compare
| sortCompare: (item1, item2) => { | ||
| // Mail set entry ID has the timestamp and mail element ID | ||
| const item1Id = getElementId(item1.mailSetEntry) | ||
| const item2Id = getElementId(item2.mailSetEntry) | ||
|
|
||
| // Sort in reverse order to ensure newer mails are first | ||
| return compare(customIdToUint8array(item2Id), customIdToUint8array(item1Id)) | ||
| }, |
There was a problem hiding this comment.
We're currently awaiting a response if this is a good enough check or if the limited 1024 ms precision would necessitate checking the mail's receivedDate separately. If not, we can check them separately as was written before.
Do not merge this until this is resolved.
| const items = await this.loadMails(lastFetchedId, count) | ||
| return { | ||
| items, | ||
| complete: items.length < count, |
There was a problem hiding this comment.
complete condition was different before because of inbox rules, should we take that into account?
| } | ||
|
|
||
| async handleEntityUpdate(update: EntityUpdateData) { | ||
| if (isUpdateForTypeRef(MailFolderTypeRef, update)) { |
There was a problem hiding this comment.
I'm thinking… we could pass labels in also directly now. We should think about it, would be a good point to do it now.
| const newMailData = await this.entityClient.load(MailTypeRef, [update.instanceListId, update.instanceId]) | ||
| // Updating the mail in-place does not require waiting for the underlying list model to finish. | ||
| // We use Object.assign here to ensure references to the mail now have the new mail data | ||
| Object.assign(mailItem.mail, newMailData) |
There was a problem hiding this comment.
I'm not sure if this optimization is worth it. What if new mail has fewer fields assigned than the old one (probably not possible yet but could). I prefer to not mutate globally-referenced instances without reason (even though that's the case for mails right now).
I also don't understand how it would trigger a list update.
There was a problem hiding this comment.
This is not an optimization. We had an issue where mails were not always being updated (for example marking mails as read/unread).
Maybe we can look at this one together.
| } | ||
|
|
||
| onSingleExclusiveSelection(mail: Mail) { | ||
| this.listModel.onSingleExclusiveSelection(assertNotNull(this.getLoadedMail(mail))) |
There was a problem hiding this comment.
Shouldn't we pass around LoadedMail now?
There was a problem hiding this comment.
LoadedMail is sort of an internal detail as we needed some sort of tuple for Mail/MailSetEntry. Do we want to expose this?
There was a problem hiding this comment.
My initial understanding was that we would have two list models (with maybe similar interfaces?): one for individual mail set entries and one for conversations.
I don't have any issue with exposing it btw.
Also tweak the timestamp check slightly; we can return the difference, since all sort cares about is if it is >0 or <0, not =1 or =-1 If this check is not necessary, this commit can be dropped.
paw-hub
force-pushed
the
8247-refactor-non-conversation-list
branch
from
79415a6 to
6f711f6
Compare
This removes MailViewModel having to keep track of MailSetEntry<->Mail, and it removes a lot of legacy code for getting Mail vs MailSetEntry, as all of the implementation details for getting mails are now in MailListModel. Additionally, everyone uses mailsets, so the legacy code is no longer necessary.
Closes #8247