Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use custom ListModel for getting mails #8308

Open
wants to merge 5 commits into
base: dev-mail
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
12 changes: 1 addition & 11 deletions src/common/misc/ListElementListModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export class ListElementListModel<ElementType extends ListElement> {
// Wait for any pending loading
return this.listModel.waitLoad(() => {
if (operation === OperationType.CREATE) {
if (this.canInsertEntity(entity)) {
if (this.listModel.itemWithinLoadedRange(entity)) {
this.listModel.insertLoadedItem(entity)
}
} else if (operation === OperationType.UPDATE) {
Expand All @@ -54,16 +54,6 @@ export class ListElementListModel<ElementType extends ListElement> {
}
}

private canInsertEntity(entity: ElementType): boolean {
if (this.state.loadingStatus === ListLoadingState.Done) {
return true
}

// new element is in the loaded range or newer than the first element
const lastElement = this.listModel.getLastItem()
return lastElement != null && this.config.sortCompare(entity, lastElement) < 0
}

async loadAndSelect(
itemId: Id,
shouldStop: () => boolean,
Expand Down
11 changes: 11 additions & 0 deletions src/common/misc/ListModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import stream from "mithril/stream"
import { ListFetchResult, PageSize } from "../gui/base/ListUtils.js"
import { isOfflineError } from "../api/common/utils/ErrorUtils.js"
import { ListAutoSelectBehavior } from "./DeviceConfig.js"
import { OperationType } from "../api/common/TutanotaConstants"

export type ListModelConfig<ItemType, IdType> = {
/**
Expand Down Expand Up @@ -577,6 +578,16 @@ export class ListModel<ItemType, IdType> {
const id2 = this.config.getItemId(item2)
return this.config.isSameId(id1, id2)
}

itemWithinLoadedRange(entity: ItemType): boolean {
paw-hub marked this conversation as resolved.
Show resolved Hide resolved
if (this.state.loadingStatus === ListLoadingState.Done) {
return true
}

// new element is in the loaded range or newer than the first element
const lastElement = this.getLastItem()
return lastElement != null && this.config.sortCompare(entity, lastElement) < 0
}
}

export function selectionAttrsForList<ItemType, IdType>(listModel: Pick<ListModel<ItemType, IdType>, "areAllSelected" | "selectNone" | "selectAll"> | null) {
Expand Down
303 changes: 303 additions & 0 deletions src/mail-app/mail/model/MailListModel.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,303 @@
import { ListFilter, ListModel } from "../../../common/misc/ListModel"
import { Mail, MailFolder, MailFolderTypeRef, MailSetEntry, MailSetEntryTypeRef, MailTypeRef } from "../../../common/api/entities/tutanota/TypeRefs"
import {
CUSTOM_MAX_ID,
customIdToUint8array,
deconstructMailSetEntryId,
elementIdPart,
getElementId,
isSameId,
listIdPart,
} from "../../../common/api/common/utils/EntityUtils"
import { EntityClient } from "../../../common/api/common/EntityClient"
import { ConversationPrefProvider } from "../view/ConversationViewModel"
import { assertMainOrNode } from "../../../common/api/common/Env"
import { assertNotNull, compare } from "@tutao/tutanota-utils"
import { ListLoadingState, ListState } from "../../../common/gui/base/List"
import Stream from "mithril/stream"
import { EntityUpdateData, isUpdateForTypeRef } from "../../../common/api/common/utils/EntityUpdateUtils"
import { OperationType } from "../../../common/api/common/TutanotaConstants"

assertMainOrNode()

type LoadedMail = {
paw-hub marked this conversation as resolved.
Show resolved Hide resolved
mail: Mail
mailSetEntry: MailSetEntry
}

/**
* Handles fetching and resolving mail set entries into mails as well as handling sorting.
*/
export class MailListModel {
// Id = MailSetEntry element id
private readonly listModel: ListModel<LoadedMail, Id>

// keep a reverse map for going from Mail element id -> LoadedMail
private readonly mailMap: Map<Id, LoadedMail> = new Map()

constructor(
private readonly mailSet: MailFolder,
private readonly conversationPrefProvider: ConversationPrefProvider,
private readonly entityClient: EntityClient,
) {
this.listModel = new ListModel({
fetch: async (lastFetchedItem, count) => {
const lastFetchedId = lastFetchedItem?.mailSetEntry?._id ?? [mailSet.entries, CUSTOM_MAX_ID]
const items = await this.loadMails(lastFetchedId, count)
return {
items,
complete: items.length < count,
Copy link
Contributor

Choose a reason for hiding this comment

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

complete condition was different before because of inbox rules, should we take that into account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Oops!

}
},

loadSingle: (listId, itemId) => this.loadSingleMail([listId, itemId]),

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))
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


getItemId: (item) => getElementId(item.mailSetEntry),

isSameId: (id1, id2) => id1 === id2,

autoSelectBehavior: () => this.conversationPrefProvider.getMailAutoSelectBehavior(),
})
}

get items(): Mail[] {
return this.loadedMails().map((mail) => mail.mail)
}

get loadingStatus(): ListLoadingState {
return this.listModel.state.loadingStatus
}

get stateStream(): Stream<ListState<Mail>> {
return this.listModel.stateStream.map((state) => {
const items = state.items.map((item) => item.mail)
const selectedItems: Set<Mail> = new Set()
for (const item of state.selectedItems) {
selectedItems.add(item.mail)
}
const newState: ListState<Mail> = {
...state,
items,
selectedItems,
}
return newState
})
}

isLoadingAll(): boolean {
return this.listModel.state.loadingAll
}

isItemSelected(mailId: Id): boolean {
const loadedMail = this.mailMap.get(mailId)
if (loadedMail == null) {
return false
}
return this.listModel.isItemSelected(getElementId(loadedMail.mailSetEntry))
}

getMail(mailId: Id): Mail | null {
return this.getLoadedMail(mailId)?.mail ?? null
}

getMailSetEntry(mailSetEntryId: Id): MailSetEntry | null {
const { mailId } = deconstructMailSetEntryId(mailSetEntryId)
return this.getLoadedMail(mailId)?.mailSetEntry ?? null
}

loadAndSelect(mailId: Id, shouldStop: () => boolean): Promise<LoadedMail | null> {
return this.listModel.loadAndSelect(mailId, shouldStop)
}

onSingleSelection(mail: Mail) {
this.listModel.onSingleSelection(assertNotNull(this.getLoadedMail(mail)))
}

selectNone() {
this.listModel.selectNone()
}

cancelLoadAll() {
this.listModel.cancelLoadAll()
}

async loadInitial() {
await this.listModel.loadInitial()
}

getSelectedAsArray(): Array<Mail> {
return this.listModel.getSelectedAsArray().map(({ mail }) => mail)
}

async handleEntityUpdate(update: EntityUpdateData) {
if (isUpdateForTypeRef(MailFolderTypeRef, update)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// In case labels change trigger a list redraw.
// We need to do it because labels are passed out of band and are not part of the list state.
this.reapplyFilter()
} else if (isUpdateForTypeRef(MailSetEntryTypeRef, update) && isSameId(this.mailSet.entries, update.instanceListId)) {
if (update.operation === OperationType.DELETE) {
await this.listModel.deleteLoadedItem(update.instanceId)
} else if (update.operation === OperationType.CREATE) {
const loadedMail = await this.loadSingleMail([update.instanceListId, update.instanceId])
await this.listModel.waitLoad(async () => {
if (this.listModel.itemWithinLoadedRange(loadedMail)) {
this.listModel.insertLoadedItem(loadedMail)
}
})
}
} else if (isUpdateForTypeRef(MailTypeRef, update)) {
const mailItem = this.mailMap.get(update.instanceId)
if (mailItem != null && update.operation === OperationType.UPDATE) {
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}
}

areAllSelected(): boolean {
return this.listModel.areAllSelected()
}

selectAll() {
this.listModel.selectAll()
}

onSingleInclusiveSelection(mail: Mail, clearSelectionOnMultiSelectStart?: boolean) {
this.listModel.onSingleInclusiveSelection(assertNotNull(this.getLoadedMail(mail)), clearSelectionOnMultiSelectStart)
}

selectRangeTowards(mail: Mail) {
this.listModel.selectRangeTowards(assertNotNull(this.getLoadedMail(mail)))
}

selectPrevious(multiselect: boolean) {
this.listModel.selectPrevious(multiselect)
}

selectNext(multiselect: boolean) {
this.listModel.selectNext(multiselect)
}

onSingleExclusiveSelection(mail: Mail) {
this.listModel.onSingleExclusiveSelection(assertNotNull(this.getLoadedMail(mail)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we pass around LoadedMail now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LoadedMail is sort of an internal detail as we needed some sort of tuple for Mail/MailSetEntry. Do we want to expose this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

isInMultiselect(): boolean {
return this.listModel.state.inMultiselect
}

enterMultiselect() {
this.listModel.enterMultiselect()
}

async loadAll() {
await this.listModel.loadAll()
}

reapplyFilter() {
this.listModel.reapplyFilter()
}

setFilter(filter: ListFilter<Mail> | null) {
this.listModel.setFilter(filter && ((loadedMail: LoadedMail) => filter(loadedMail.mail)))
}

isEmptyAndDone(): boolean {
return this.listModel.isEmptyAndDone()
}

async loadMore() {
await this.listModel.loadMore()
}

async retryLoading() {
await this.listModel.retryLoading()
}

stopLoading() {
this.listModel.stopLoading()
}

private getLoadedMail(mail: Id | Mail): LoadedMail | null {
if (typeof mail !== "string") {
paw-hub marked this conversation as resolved.
Show resolved Hide resolved
return this.mailMap.get(getElementId(mail)) ?? null
} else {
return this.mailMap.get(mail) ?? null
}
}

private loadedMails(): readonly LoadedMail[] {
return this.listModel.state.items
}

private async loadMails(id: IdTuple, count: number): Promise<LoadedMail[]> {
const mailSetEntries = await this.entityClient.loadRange(MailSetEntryTypeRef, listIdPart(id), elementIdPart(id), count, true)
if (mailSetEntries.length === 0) {
return []
}
const entries = await this.resolveMultipleMailSetEntries(mailSetEntries)
this.onLoadMails(entries)
return entries
}

private async loadSingleMail(id: IdTuple): Promise<LoadedMail> {
const mailSetEntry = await this.entityClient.load(MailSetEntryTypeRef, id)
const mail = await this.entityClient.load(MailTypeRef, mailSetEntry.mail)
const loadedMail = { mailSetEntry, mail }
this.onLoadMails([loadedMail])
return loadedMail
}

private async resolveMultipleMailSetEntries(mailSetEntries: MailSetEntry[]): Promise<LoadedMail[]> {
// Sort all mails into mailbags so we can retrieve them with loadMultiple
const mailListMap: Map<Id, Id[]> = new Map()
for (const entry of mailSetEntries) {
const mailBag = listIdPart(entry.mail)
const mailElementId = elementIdPart(entry.mail)
let mailIds = mailListMap.get(mailBag)
if (!mailIds) {
mailIds = []
mailListMap.set(mailBag, mailIds)
}
mailIds.push(mailElementId)
}

// Retrieve all mails by mailbag
const allMails: Map<Id, Mail> = new Map()
for (const [list, elements] of mailListMap) {
const mails = await this.entityClient.loadMultiple(MailTypeRef, list, elements)
for (const mail of mails) {
allMails.set(getElementId(mail), mail)
}
}

// Build our array
const loadedMails: LoadedMail[] = []
for (const mailSetEntry of mailSetEntries) {
const mail = allMails.get(elementIdPart(mailSetEntry.mail))
// Mail may have been deleted in the meantime
if (mail) {
loadedMails.push({ mailSetEntry, mail })
}
}

return loadedMails
}

private onLoadMails(mails: LoadedMail[]) {
for (const mail of mails) {
this.mailMap.set(getElementId(mail.mail), mail)
}
}
}
2 changes: 1 addition & 1 deletion src/mail-app/mail/view/MailListView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ export class MailListView implements Component<MailListViewAttrs> {
color: theme.list_message_bg,
})
: m(List, {
state: listModel.state,
state: listModel.stateStream(),
renderConfig: this.renderConfig,
onLoadMore() {
listModel.loadMore()
Expand Down
Loading
Loading