937959 - [email] MailAPI/model refactor

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[James Burke [:jrburke]]

In bug 918303, the use cases around 'model', MailAPI and slices was starting to get blurry, and :asuth and I talked about doing the following refactor steps to help improve that story. These may be broken out as sub-bugs at some point, but documenting it all so we do not lose it:

1) slices to become event emitters to allow multiple listeners for slice events.

2) 'model' to merge into MailAPI:
* MailAPI should be able to start up without starting up the backend as model does.
* MailAPI should be able to create a BrowseContext which would track the slices that would be reused by the front end code (so handles the slices that 'model' handles now. Multiple BrowseContext could be created.
* mail_app would create a BrowseContext from MailAPI instead of using 'model', and 'model' would go away.

3) HeadersViewSlice should be able to track a "current header", and event out when that current header value changes, along with if there is a next and previous message available for it.

This is refactoring and due to timelines, probably should not be attempted before 1.4.

Comment 1

[James Burke [:jrburke]]

Bug 962451 had an issue where a message_list card is created after the header_cursor has a slice. One option may be for whatever is new here is to allow a replay of messages for a slice for late listeners. Need to consider how much data the replay queue holds on to though, maybe just restrict to message_list listeners? That may not make sense to have it so specialized either though. Just some notes to consider later.

It would also be good to make sure activity triggers that go back to the message_list, as described in bug 962451, and a test confirming the behavior even better.

Comment 2

[Dylan Oliver [:doliver]]

[priority] --> tracking-b2g:+ conversion

Comment 3

[Dylan Oliver [:doliver]]

Removing from tracking -- this is not high priority on its own, but we'll pick it up at some point as part of another feature that would benefit from it.

Comment 4

[James Burke [:jrburke]]

Bug 1119161 comment 3, item 2), highlights a desire to have multiple header_cursor instances to the BrowserContext object to fix some awkwardness around message_list and message_list_search sharing a header_cursor, so be sure to factor that into the design.

Comment 5

[James Burke [:jrburke]]

I have started to look at this, but focusing on this intermediate refactor point: make model and header_cursor classes that can be instantiated.

This allows different sets of cards to operate on different model and headerCursor instances. This tests how it feels to go to that instantiated model instead of using a well-known module ID as the coordination point.

This means that cards need to pass around state data, received by other cards in the `onArgs` hook in our cards instances.

This is being prototyped in a `email-model-instance` branch in my fork of gaia, current diff:

https://github.com/mozilla-b2g/gaia/compare/master...jrburke:email-model-instance

What is there basically works, but I still need to run the tests and likely might need to do some tweaks in there.

Basic explanation of the changes:

model.js -> model_create.js. Exports a modelCreate function that can be used to create new instances. `modelCreate.defaultModel` is a default model instance.

header_cursor.js, name kept, but now exports a HeaderCursor constructor function that accepts a model object as its only argument. CurrentMessage constructor is now at HeaderCursor.CurrentMessage, since it really just an internal object created by HeaderCursor, but the unit test wants to new up a specific CurrentMessage instance, so wants to be publicly accessible in some form.

Either config.js or mail_app.js inits modelCreate.defaultModel. Due to the startup cache, in the no-cache case, config.js inits the model to figure out if there are accounts so that enough state is known to make mail_app.js to choose the startup UI efficiently. Otherwise, if the startup cache is usable, model init does not happen until mail_app.js, after initial UI view is chosen.

Because of this type of coordination, it felt useful to have a default `model` instance, and that drove the creation of modelCreate.defaultModel. It also helped the unit tests, but those could be managed differently if they were the only things that benefited from the defaultModel. The startup coordination was the main driver for it.

Once the model instance is created, it is passed to cards via `onArgs`. message_list and message_reader also accept headerCursor instances, but can create new ones if not passed.

This now allows message_list and the search version of message_list to have different headerCursors, and even separate model objects, although for now they share the same model object.

My next step in that branch is to separate the search-only stuff from message_list to its own card definition, and at that time, HeaderCursor can be changed to drop tracking searchMode.

I tried starting with an approach where each card would only try to use a specific slice it needed, but it seemed like a regular concern for the card to also care about the account, and to be able to react to changes into accounts and folder slices.

So it seemed useful to create an object instance that does not change, but has sub-pieces that do change and consumers of the object instance can watch for changes on those sub-pieces. As opposed to only getting the sub-piece, but somehow needing some signals to discard those sub-pieces when they were no longer relevant.

Some of this is driven by us favoring being able to create the card view and have it animate in even before the data is available. I think this is a good design choice given the user perceived speed benefits, just listing the design forces that argue for this sort of stable model handle the card can receive before the model data is actually loaded.

Otherwise, we could decide to wait until the data is available, then only give the sub-piece of data to the card.

In that approach, there would still be a larger invalidation/update of that whole sub-piece: for example, if folder_picker changes the account, the message_list needs to then load the new account's selected folder.

So to manage that coordination, particularly since we want cards to be instance based, would mean using something like the `model` object to coordinate changes.

Which I believe is what the goal of a BrowseContext object would be, although I am now wanting a better name for it than BrowseContext, as it "browse" feels too generic.

What I am hoping to do as next steps, absent any discussion/feedback to the contrary:

* I proceed with this pathway to get to cards tracking instances for model data, maybe even landing something for it once message_list/search is separated, since that separation will likely help the front end with introducing a convo_list card.
* The conversations backend changes will allow slices to use evt for multiple listeners.
* That multiple listener capability will allow us to revisit more of the internals of `model`, and we change it to use that capability.
* We then rename `model` to what BrowseContext represents, move it into gelam, and the front end now uses that instead of `model`.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Re: BrowseContext and naming, I think there's two use-cases that model addresses:
A) The propagation of account/folder changes through to the message list
B) The header cursor coordination between a "conversation/message list" card and a "conversation message list/message reader" card where we want navigation in the message reader card to be reflected in the message list for UX purposes and because of the logistical issue of the "conversation/message list" case having an explicit window of interest that needs to be updated as we move.  (Point 3 from comment 0)

I've mainly been thinking of 'B' as the use-case for BrowseContext since this concept of coordinated list/detail exists both in our card idiom and in multi-pane tablet and desktop cases.

'A' seems somewhat more app-specific.  For example, for the react.js-based desktop app, propagation of folder changes occurs through a change of URL which changes the route.  All objects are conceptually torn down (although react is potentially able to reuse stuff).  As you point out, the gaia email app does this partially because of its sophisticated lazy startup mechanism, and then we also don't want to be destroying our DOM, etc.


The 'B' case is somewhat complicated in the conversations case by the fact that messages stay in the same place in the message list because their timestamps are immutable, but conversations jump around based on who their most recent message is.  (Usually upwards in the "more recent" direction.)  What to do ends up being a UX call.  I briefly checked gmail and fastmail (neither of which is multipane; the list/readers are each fullscreen) and the results are:
- fastmail: the reader screen updates its position in realtime, so what was previously "next" or "prev" can change as new messages come in.
- gmail: at least in Firefox (which seems to sometimes be lazy about getting updates from the server): the reader screen does not update its position, so "next"/"prev" seem to remain consistent with what the user saw prior to entering the reader page.

While this ends up somewhat moot if we don't end up with next/prev buttons in our conversation UX, there's still the question of what to do when leaving the message reader and the conversation jumped off screen.  (Although this does not actually require the cursor, just for the windowed list view to know to track the conversation identity rather than the original position.)


Which is mainly to say in terms of next steps:
- I think it makes sense to split along the A/B lines and only move 'B' in to gelam.  Or be more explicit why all consumers of 'B' also want 'A'.
- I also don't have a problem also putting 'A' into gelam if you think it's generic enough or just that changes to gelam would correlate with changes to 'A' and it's less of a headache for it to live in gelam for that reason.

Comment 7

[BMO Automation]

Firefox OS is not being worked on