refactor(list): Detangle canvastable, messagedisplay, virtualtable

[castaway]

An initial start on blending the new messagelist via virtual table, into the existing MessageDisplay based "whats displayed" management:

• The MessageDisplay object is now held by the main app object (as canvastable.ts should be considered deprecated)
• MessageDisplay now stores the current state of the column sort/ order variables
• MessageDisplay has all copies of the "rows" (we do seem to have rather a lot of row list copies somehow) - ideally displayRows would be the content that actually goes onto the page, eg [items]=messagedisplay.displayRows in the template, however I am not yet understanding the purpose of the rowsSubject (another copy?) not the debouncedRows$ which isn't referenced anywhere?
• enrichRow created/moved into the MessageDisplay
• Re-introduces the bulk-download, using the renderedRange change to fetch the content of the scrolled-to messages if not previously fetched
• renderedRange code, since it already fetches the messagecontents, assigns the text to the plaintext field and calls enrichRow to assign it
• updateRows/enrichRows - the former is gone, the latter now only exists to manage the initial load of rows, ideally it should all be handled by renderedRange updates

Still Todo/Understand:

• changed canvastable.rows.removeMessages calls to messagedisplay.removeMessages, but - how to actually update what is displayed in the list?
• Ditto singleMailViewerClosed / clearOpenedRow - the MessageDisplay can manage the row, how to attach this to the models the virtualtable uses?
• app.rowSelected + models for message selection/filtering etc should, imo, also live in the MessageDisplay class
• Saving of column widths into preferences
• Updating column widths when window is resized (proportionally)
• Column style / naming management (the row data should contain "from" and "to" fields, it querying the app.selectedFolder to change its content is.... ugly)

Errors/Issues:

• renderedRange - somehow I had to change the default start/end to 0,10 (from 0,0), and now its not filling out the rest of the available height with more data - where is that triggered?

[shadowbas]

@castaway , the issue you are experiencing is likely due to the early return. In the virtual table we @Output events whenever the render range changes. Only the virtual scroll knows this and we pass it as an event to the component that manages the fetching of state which in our case is the app.component.ts.

https://github.com/runbox/runbox7/blob/shadowbas/feat-accessible-table/src/app/app.component.ts#L1499

EDIT: Another possibility is that between the renderRangeChange event and the rowsSubject emit we do not manage the state correctly or stop somewhere in that flow.

[shadowbas]

This returns a promise. This is necessary for the async filter in the template to result in a redraw once the messageText is fetched.

I believe https://github.com/runbox/runbox7/pull/1712/files#diff-884f7f49640e5923f6bcac4c51d90340330a178f662defbe61e5f5aac1c512deR1618 is not a promise.

[castaway]

Hmm have run it checking this, and its not returning here (the messagedisplay does exist) .. still baffled how renderedRange(Stream) changes

[castaway]

gah i dunno why this comment was here.. I was referring to a different one!

[shadowbas]

The following callback is used in the template and listens for an event from the virtual scroll table component. That component emits this event.

https://github.com/runbox/runbox7/pull/1712/files#diff-884f7f49640e5923f6bcac4c51d90340330a178f662defbe61e5f5aac1c512deL1610

[shadowbas]

Just to be extra clear. Child component state should only be communicated to parent through events (@Output) and not through mutation.

This is a principle common in most frontend frameworks.

Yes one can deviate from this but often that breaks the isolation principle, making a component tangled with state management.

[shadowbas]

It is bad practice to pass the this of a complete component to some other module.

[castaway]

I agree! Your code does it a bunch, I have been trying to figure out how not to do it

[shadowbas]

Simplest is to have enrichRow return and then mutate the component state. We we should not have a different module mutate state on the app component.

Another solution is to have the state live in another module/class (isolated) and be able to get a clone or do a prototype inheritance with (Object.create) to prevent mutating the isolated state.

// Encapsulated in the messages module/service
await this.messages.enrichRow(messageIdMap.get(msg['mid']))
this.messages = this.messages.list();

// or have the state in app.
this.messages[index] = await this.messages.enrichedRow(msg)