Closed Bug 918303 Opened 8 years ago Closed 8 years ago

[User Story] Quick Advance Though Emails

Categories

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

ARM
Gonk (Firefox OS)
enhancement
Not set
normal

Tracking

(b2g-v1.3 wontfix, b2g-v1.3T wontfix, b2g-v1.4 fixed)

VERIFIED FIXED
1.3 C1/1.4 S1(20dec)
Tracking Status
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- fixed

People

(Reporter: pdol, Assigned: gaye)

References

Details

(Keywords: feature, Whiteboard: [ucid:Productivity11, 1.4:p1, ft:productivity][p=5])

Attachments

(1 file, 2 obsolete files)

User Story:

As a user, I want to be able to quickly browse through my emails (body view) without needing to go back to the list view to select the new email to view, saving me time. 


Acceptance Criteria:

1. While viewing a particular email, I can advance to the next email using a particular action, taking me to the body view of the next email in the list
2. If I am viewing the last email in a particular list, the action to advance to the next email gives me an indication that I have reached the end of the list
3. While viewing a particular email, I can advance to the previous email using a particular action, taking me to the body view of the previous email in the list
4. If I am viewing the most recent email in a particular folder, the action to advance to the previous email gives me an indication that I have reached the top of the list
Assignee: nobody → gaye
Based on the message reader now being able to scroll through the contents of the folder, I think we want the message reader to have access to a slice too.  And since the UX spec is calling for the message list updating to ensure the most recently read message was in the list, I think we probably want to be using the same slice for both.

The wrinkle is that slices weren't 100% designed to cover this case, part of the issue is that we don't have proper multi-recipient event listeners, part is that I put 'element' attributes on the MailAPI-produced objects so we could link them to their DOM nodes and we actually use them, making reuse odd.


Right now in the message reader we have a fairly horrible hack where we end up with:

- header: An 'inert' copy of the the message header we are dealing with.  Although it thinks it is owned by the slice the thing it is being cloned from, it is not known to the slice and so it does not receive any updates.  We did this because our logic relies on the MailPeep.onchange (which can only have a single listener) and MailPeep.element place to stash the reference to the DOM node.  The upside to the use of 'element' mainly just lets us avoid having N 'onchange' closures.  It's worth noting that the way the copy works, all of those MailPeeps are live/updating.  As contacts are added/removed, we will get onchange events.  But the header is otherwise not live.  The body retrieved from the header is inherently live because it results in its own subscription.

- hackMutationHeader: This is the live header that the slice knows about so it can get updates.  This exists so that our star-toggling can be up-to-date.  We just look at it and use it to poke at the message.


I see a few ways to deal with this:

A) Fix up the event listeners to not just be attributes, but to use a fancier event model, especially now that we have a canonical one in the codebase.  Use exactly the same objects for all purposes.  Do this via:
A1a) Use EventEmitter, and use closures on everything to replace use of "element".  It's not that many closures, but we do need to be careful about cleanup of the listeners.
A1b) Use EventEmitter, Maps to map from the object to the DOM node.  Careful about cleanup.
A1c) Re-query to find the nodes from the DOM based on a unique ID whenever we want to update them.  This feels wasteful when we could be O(1)-ish, but maybe it doesn't matter.

A2) Implement a DOM bubbling style of event handling so that changes on the MailPeep instances/etc. bubble up to the header.  This was we're only adding a small number of listeners so it's less work to remove them.  We'd have to use a Map or the re-querying strategy to find the specific DOM node, though.

B) Improve our makeCopy/clone logic so that it still gets live updates.  The message reader clones the header as it needs to, then kills it when it's done.


Having described them, I'm leaning towards 'B' then 'A2', but I hard a hard time with this implementation decision before and am still having a hard time deciding what's best, so I would definitely like other input.


In terms of Gareth's IRC question about finding the next/previous, I would lean towards having the message list and message reader communicate via their own simple calls since that will also allow unit testing in isolation.  So:

- reader -> list: move(currentHeader, 'newer' or 'older') => get back a new header or null if the reader should just give up and close itself.  This is what would cause the list to trigger the "load more messages" logic, transparently.
- list -> reader: moveFlags({ newer, older }).  This would be sent unsolicited when a sync completes, such as cronsync running, or the automatically triggered "load more messages" logic.

And because we do have that lazy loading stuff with cards, probably the reader would want to register with the list asynchronously once the list comes into existence.
In the spirit of "cards communicating with their own simple calls" approach: 

I would be OK for now with reader using evt to broadcast and listen for messages, with the message_list doing the same for broadcasting list position. So something maybe like:

reader:

evt.on('messagePosition', function (messageSuid, navSuids) {
  //if have navSuids, then show button state accordingly
});


// emits this event when wanting to know more about the
// message position
evt.emit('messageListPosition', messageSuid);

message_list:

evt.on('messageListPosition', function(messageSuid) {
  // checks to see if there are messages before and after,
  // then sends:
  evt.emit('messagePosition', messageSuid, {
    prevMessageSuid: ...,
    nextMessageSuid: ...
  });
});

message_list could keep around last known messageSuid if it knows it is at a boundary. And button clicks could evt out asking for the messageHeader for that suid, with message_list answering.

May need some more unique names, and a bit more thought, just thinking out loud. For direct jumps into the email app from a notification, where a message_list card may not yet exist, then the next/prev would just be disabled until message_list got created.

Also, I made evt sync notify listeners for event loop speed concerns, but at some point may want to visit making it more async if the above approach does not work out well. Just mentioning it as something I would also be willing to change for 1.3 if it allowed some better use of this pattern.
Blocks: 925022
No longer blocks: 925022
Thanks for the ideas asuth and jrburke! Concerning the interface between the MessageListCard and MessageReaderCard...

We know that the MessageListCard needs to know about the next and previous messages in order to use the grey arrows when and if we reach a folder bound. Therefore, I think that it makes most sense to add previousSuid and nextSuid to the args we pass to the MessageReaderCard. Then I think it'd be simple when one of the arrows gets clicked for the MessageReaderCard to emit an 'advance' message with the id of the card to advance to. For instance...

// MessageReaderCard
MessageReaderCard.prototype.onUp = function(event) {
  // Advance to the previous message.
  evt.emit('advance', this.previousSuid);
};
MessageReaderCard.prototype.onDown = function(event) {
  // Advance to the next message.
  evt.emit('advance', this.nextSuid);
};

// MessageListCard
evt.on('advance', function advance(messageSuid) {
  // Scroll so that the message is showing.
  this.scrollNode.scrollTop = ...;

  Cards.pushCard('message_reader', 'default', 'animate', {
    header: undefined,
    messageSuid: messageSuid,
    previousSuid: ...,  // Figure out what these are
    nextSuid: ...       // by looking in the DOM
  });
});

Thoughts?
(In reply to Gareth Aye [:gaye] from comment #3)
> We know that the MessageListCard needs to know about the next and previous
> messages in order to use the grey arrows when and if we reach a folder
> bound. Therefore, I think that it makes most sense to add previousSuid and
> nextSuid to the args we pass to the MessageReaderCard. Then I think it'd be
> simple when one of the arrows gets clicked for the MessageReaderCard to emit
> an 'advance' message with the id of the card to advance to. For instance...

Thanks for the example snippets!

I do like the idea of having the list drive the navigation since it does simplify the shared data structure access.  We do want to make sure that our onScroll logic still fires appropriately, including the 'automatically trigger "load more messages"' stuff as discussed on the e-mail thread in my message of 10/31.

The trickiness in my mind is that checkpointing the suid's is potentially dangerous.  Messages can appear and disappear at random from the perspective of the message reader.  (The most likely thing to trigger is another IMAP client doing stuff, like Thunderbird running its filters/bayesian spam classifier and discarding/moving things.)  It makes it very likely that the suid's will become invalid in the real world.

Perhaps we endow the message list with a sense of its "current" message and we do .emit('advance', -1) for previous and .emit('advance', 1) for next.

When the message list triggers the message reader (or is triggered by the message reader when directly opened from an activity), it saves off a reference/index position to that message.  If that message itself gets removed by the back-end, we arbitrarily choose the preceding or succeeding message to be the new current message.  When we get the 'advance' notification, we always fetch the header with the given index offset.

We could then do hasPrevious/hasNext with boolean values and let those simply become stale if you don't want to send the unsolicited updates.  The buttons lying a little bit are going to be less broken than us trying to show deleted messages/skipping new messages, etc.  I would lean towards the live updates since we have a 'tell' mechanism and we would be maintaining the current message cursor anyways and we would probably want to unit test doing the right thing there anyways.
(In reply to Andrew Sutherland (:asuth) from comment #4)

> Perhaps we endow the message list with a sense of its "current" message and
> we do .emit('advance', -1) for previous and .emit('advance', 1) for next.
> 
> When the message list triggers the message reader (or is triggered by the
> message reader when directly opened from an activity), it saves off a
> reference/index position to that message.  If that message itself gets
> removed by the back-end, we arbitrarily choose the preceding or succeeding
> message to be the new current message.  When we get the 'advance'
> notification, we always fetch the header with the given index offset.

Sounds good to me and I think you bring up a good point about hanging onto bad suids.

> We could then do hasPrevious/hasNext with boolean values and let those
> simply become stale if you don't want to send the unsolicited updates.  The
> buttons lying a little bit are going to be less broken than us trying to
> show deleted messages/skipping new messages, etc.  I would lean towards the
> live updates since we have a 'tell' mechanism and we would be maintaining
> the current message cursor anyways and we would probably want to unit test
> doing the right thing there anyways.

This sounds good to me.
Pointer to Github pull-request
Comment on attachment 828258 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13297

Still a work in progress, missing code to (1) scroll the MessageListCard as we wade through emails and (2) send live hasSibling updates from the MessageListCard to the MessageReaderCard, but worth taking a mid-sprint look I think. Thanks asuth!
Attachment #828258 - Flags: feedback?(bugmail)
Comment on attachment 828258 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13297

Looking good!  My feedback is on the pull request.  I had a few questions for jrburke about the front-end stuff, plus he should take a look too for all of the front-end lazy-loading strategery.
Attachment #828258 - Flags: feedback?(bugmail) → feedback?(jrburke)
Pointer to Github pull-request
Attachment #828796 - Attachment is obsolete: true
Attachment #828258 - Flags: review?(bugmail)
Attachment #828258 - Flags: review?(jrburke)
Attachment #828258 - Flags: review?(bugmail)
Attachment #828258 - Flags: feedback?(jrburke)
Comment on attachment 828258 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13297

Did a review pass documented here:
https://github.com/mozilla-b2g/gaia/pull/13297#issuecomment-28106364

I think there is enough uncovering of use case factors, like the notification pathway, to warrant another pass at the code as mentioned in the review comment. Please feel free to flip review back to me when you want me to look again.
Attachment #828258 - Flags: review?(jrburke)
I should have posted my review comment here, to keep it together with the larger design comments that :asuth had in Comment 1, so moving it back here, particularly after :asuth's comment here:

https://github.com/mozilla-b2g/gaia/pull/13297#issuecomment-28110464

So, talking about:

1) What changes in general may make sense for slices?
2) What are the clearly defined roles for the 'model' module and the slices?

On 1), at first glance, I like the idea of slices becoming full event listeners instead of relying on single property listeners. This would have avoided some conflict I felt before about reusing a slice for two card UI controller purposes. Proper .die() destruction of a slice still needs some thought using the same slice for multiple UI controller purposes. But that is a separate problem from allowing multiple listeners.

I am a bit hesitant to move DOM tracking into a slice though. I like slices to be just pure data entities. I can see a separate mixin that can be added to a slice to enable DOM node tracking and two-way data binding, and slices that are event emitters would help make that option more possible. But first thought is that the DOM tracking should be something separate from pure slice code.

Which leads to 2) the roles of 'model' and slices. There is some delayed loading via indirection that model provides, but also I think there is a set of UI data that makes sense to track in front end code. So more than a slice, but not part of the slice because it is specific to the specific kind of UI.

The model/messages_cursor felt like one of those items: it is specific to UX needs, tracking "current" message, and that current message can be set outside of the slice use directly, and knowing if a "next" or "previous" (or what to do if there is not a next/prev, like whether to grow the slice or not) could be separate concerns from a slice. While a single cursor with next/prev state indicators seems generic enough, I can see it still a factor of the UI: the UI could decide to group messages differently, even doing a primitive "show all related conversations in current slice" in UI via brute force slice items iteration. Or we may have two cursor models using the same slice. Maybe though that cursor is generic enough to keep on the slice.

This gets into how I have been viewing slices: they are like the proxies we have to the backend data. In a traditional server-backed web app, slices would be doing network calls under the covers, maybe via sockets, to a backend.

So, they are more expensive entities to use. We would not want multiple instances of proxy objects if they represented the same data, to cut down on server communication overhead, but rather, favor just using one and sharing it for all the purposes that needed the same data.

Since the slices need to go over the postMessage bridge, slices felt like more expensive property objects, and 'model' was a way to hold on to the ones that were needed for the, but also gave the UI a well-known endpoint (by asking for the 'model' module) to get a handle on the ones that all wanted the same slice data.

This may not be an accurate view of the goal of slices though. It may be worth having a more realtime talk to sort things out, then post back with any resolved plan.
:asuth and I talked about longer range refactor goals, and I documented the main points in bug 937959. 

We do not think we should attempt those changes for 1.3, or for this ticket. So for this ticket, let's try with the approach mentioned in the previous feedback in the pull request, and if it falls short for something, we can use bug 937959 as guidance on what directions to pick, but we should keep the changes small for 1.3 timeline purposes.
Whiteboard: [ucid:Productivity11] → [ucid:Productivity11, 1.3p2, ft:productivity]
Whiteboard: [ucid:Productivity11, 1.3p2, ft:productivity] → [ucid:Productivity11, 1.3p2, ft:productivity][1.3:p2]
Comment on attachment 828258 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13297

Hi jrburke!

I have another patch for you to review. There are some elements notably missing. In particular...

- Scroll the message list card when it receives currentMessage messages
- messagesSlice.onsplice should choose a new currentMessage if ours got taken away
- messagesSlice.onsplice should send next and previous data since it may have changed
- When toggling the message reader header gets messed up since we don't erase the old data before writing new data

That being said, it passes some basic acceptance tests and the architecture is updated to look more like what you suggested. The last pieces are mostly in the third decimal place...
Attachment #828258 - Flags: review?(jrburke)
Comment on attachment 828258 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13297

I left some comments in the pull request, most notable one is probably:
https://github.com/mozilla-b2g/gaia/pull/13297/files#r7849637

Clearing review flag for now as this is still in process, feel free to flip review again as it moves along.
Attachment #828258 - Flags: review?(jrburke)
Attachment #828258 - Flags: review?(jrburke)
The patch is ready to land in my mind. Passing it over to jrburke...

James - If it looks good to you, please r+ and merge for me!
Comment on attachment 828258 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13297

Finished a review, notes in the pull request. There are enough changes, some because this cursor object gives us a a cleaner model API for message_reader to use, that I would like to do another review pass after feedback is addressed. It is coming along nicely though.
Attachment #828258 - Flags: review?(jrburke)
Whiteboard: [ucid:Productivity11, 1.3p2, ft:productivity][1.3:p2] → [ucid:Productivity11, 1.4:p1, ft:productivity]
I did a pass at moving all messagesSlice work in message_list and message_reader into header_cursor. The goal of this was to address the following issues:

* if message_list and header_cursor have their own messagesSlices, they will not align on where in the slice they are at.
* Searching used its own special slice, which would cause problems too.
* notification entry point when email was in background in another folder showing a message_reader for a message in that non-inbox folder would have caused problems.

Now header_cursor manages the creation of messagesSlice, and it effectively wraps that instance in an event emitter, by using itself, header_cursor, as the event emitter. It does this by emitting events that are prefixed with a `messages_` key. To simplify wiring up those listeners, the two cards and header_cursor now have some method names with that same prefix. So while it is not the standard naming convention, it is done to simplify event wiring.

The work is in this branch (compare view):

https://github.com/jrburke/gaia/compare/bug-918303-mod

Specifically this commit:

https://github.com/jrburke/gaia/commit/c49c6eefe580d9c5b4d1a45870a4e0904e9e6932

The notification entry point now works better, and the usual next/prev navigation works. Some more testing to be done though:

* search use case has issues. However, I think it is muddied up by the current html search failure, so fixing that and retesting will help isolate the issue.
* When a message is tapped in message_list, on the very first message_reader instance showing, it is slow enough that we can see the message_list jump a bit. I believe this is because the "scroll to" logic could use a tweak, as in not try to jump in that case, or be more lenient about what it considers to be "in view".
* Would be good to test the "syncFailed" case when syncing from the message_list.
* Need to add a note to header_cursor that consumers of the messageSlice, like message_list, may add extra data to slice items. In the case of message_list, it adds an `element` member to each item. So, just to call out that is a shared data space.

This is as far as I could get, and will not be able to look at this for about a week, so snapshotting the progress. :gaye, feel free to pick this up and pull that one commit if you like. Unfortunately, the github UI will not allow me to select gaye/gaia as a pull target for some reason. Otherwise I would have just done a pull request.
Working now on adding some tests for those cases. Also (about the scroll logic) I think I'll go ahead and check whether the scroll position is permissible rather than assuming that we have to scroll.
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
Whiteboard: [ucid:Productivity11, 1.4:p1, ft:productivity] → [ucid:Productivity11, 1.4:p1, ft:productivity][p=13]
Whiteboard: [ucid:Productivity11, 1.4:p1, ft:productivity][p=13] → [ucid:Productivity11, 1.4:p1, ft:productivity][p=5]
Comment on attachment 8350212 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/13297

r+ with the following addressed (thanks for already addressing the other comments):

1) Picking up a fix for the cache wipe of the message_list messages, see:
https://github.com/mozilla-b2g/gaia/pull/13297/files#r8493164

2) The syncfailed test seems malformed, and I do not think it adds any new test info for this pull request, so you could remove it and the associated fakeimap.js change:
https://github.com/mozilla-b2g/gaia/pull/13297/files#r8493363

3) I would encourage not placing the next/prev styles and images in the shared/ location since email does not own that area, and adding styles to a shared area could have unintended consequences. If they are kept in there at the very least it should get a review check from an owner of the shared/ area.
Attachment #8350212 - Flags: review?(jrburke) → review+
Landed on master https://github.com/mozilla-b2g/gaia/commit/961e8028850693d61317c968bd078e74fb08dda9. Thank you thank you thank you thank you asuth and jrburke for all of your help!
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
@Edward,
Could I have your help?
I am busy on other functional team's stuffs.

Please help verify this feature by using following test case.
- https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=50&sortfield=created_on&sortdirection=desc&filter-tag=2334&filter-productversion=164

Thanks!
Flags: needinfo?(edchen)
Got it. I'll start testing this feature.
Flags: needinfo?(edchen)
Thanks for Edward's help.
Marked it as "VERIFIED"

=== Quote Edward's information ===
* Build Information:
  - Device 1 - Helix
   * Gaia      dfae3744607257206e37483dc3f431108baf70fb
   * Gecko     https://hg.mozilla.org/mozilla-central/rev/31113754db3b
   * BuildID   20140223160201
   * Version   30.0a1
   * ro.build.version.incremental=eng.wanglei.20131212.005845
   * ro.build.date=Thu Dec 12 00:58:58 CST 2013

  - Device 2 - Buri
   * Gaia      dfae3744607257206e37483dc3f431108baf70fb
   * Gecko     https://hg.mozilla.org/mozilla-central/rev/c8bea55437c1
   * BuildID   20140303160208
   * Version   30.0a1
   * ro.build.version.incremental=eng.archermind.20131114.105818
   * ro.build.date=Thu Nov 14 10:58:33 CST 2013

* Test Result:
  - Manual test (24 passed, 4 fail)
Status: RESOLVED → VERIFIED
Severity: normal → enhancement
Depends on: 989116
You need to log in before you can comment on or make changes to this bug.