Closed Bug 881469 Opened 7 years ago Closed 6 years ago

[MMS] Implement Navigation object

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.0, tracking-b2g:backlog, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S2 (23may)
feature-b2g 2.0
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: jugglinmike, Assigned: julienw)

References

Details

(Whiteboard: [p=3][comms-triaged])

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
azasypkin
: review+
steveck
: review+
Details | Review
Throughout the application, navigation is achieved by simply setting `window.location.hash` and relying on the `hashchange` event handler to update state accordingly.

While this approach usually works, it makes a potentially false assumption: that the application is not *already* viewing the specified hash. In those cases, the `hashchange` event handler will not be triggered, and the application state will not update.

This is particularly dangerous when entering the application through indirect means (i.e. through MozActivities like "share" and "new").

Implement a generic `navigate` method that detects the current state of the document hash, and manually triggers the expected behavior in cases where no `hashchange` event would otherwise fire.
I'd like to move away from getting window.location.hash to know the view we're in. I see no particular issue with setting it for changing views (but _not_ to eg trigger edit mode), apart the issue you're explaining here.

Do you think that moving to pushState/replaceState/onpopstate could bring some improvements ? One of the main improvement I can see is to use a structured object as a state, instead of using a hash. Could bring some simplifications to keep the current thread id + the current view there.
Using the history API would certainly save us from that string parsing. We will lose out on the ability to share URLs to states within the application, however. I don't know enough about Gaia's overall architecture to know if this is a requirement. Currently, is there a use case to link to application states?

In other words, is there any part of Gaia that recognizes the hyperlink "app://sms.gaiamobile.org/#new" and forwards appropriately?
Nope, that doesn't exist and I'm not aware of any plan about doing this.
Please renominate when we have a real world user scenario.
blocking-b2g: leo? → -
I think we'll do this once we start the 1.2 development cycle... unless this bites us too much.
Im gonna take this as well after talking with Julien. It's gonna be a part of the work started here 880115.
Assignee: mike → fbsc
Depends on: 880115
structure suggestion for the state object:

{
  view: 'thread' / 'threadlist' / 'newthread' / falsy,
  activity: falsy or { body, number, contact },
  threadId: id if view === 'thread' / falsy otherwise
}

I think we should use only pushState; history.back would trigger onpopstate which would let us move to the previous view.

Using pushState doesn't trigger "popstate" though, so I don't know what the best solution would be to trigger the controller. Maybe we should not use pushState at all and rather use our own state/navigation controller (which could use pushState behind the scene).

Mike, I'd value your input here.
I agree that we'll still need a "navigate" method.

The "view" information really makes more sense as part of a URL. We talked about Gaia not using URLs in comment 2 and comment 3. Ignoring my own reservations about that fact, I think that it would still be worthwhile to follow the paradigm. Using the URL for its intended purpose will:

- increase the "webiness" of our source code
- lead to a more intuitive experience when running in FF Nightly (the URL will update as you move through the application)

Both these advantages are pretty minor, but the difference in implementation is trivial:

- the application's single call to `history.pushstate` specifies the view name in the third "URL" argument
- the `onpopstate` handler inspects the URL for the view name

What do you think, Julien?
I'd think that using a property in the state object is easier, faster, simpler, but I value the webbiness of the URL too.

The URL is especially important when you load it, but not really when you stay inside the app. How about keeping that "view" property, but reflecting it in the URL ? This is quite useless (for now at least) but would stay webby. At the start of the application, we could inspect the URL and replaceState the appropriate state based on that URL.

Is it the best of both worlds ?
Summary: [MMS] Navigation logic is brittle → [MMS] Implement Navigation object
Blocks: 912012
blocking-b2g: - → koi?
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
This bug could be a great one for landing during Oslo WW, due to we could discuss together the details about it.
I think the Oslo WW is really targeted at 1.2 and we should not do this now.

Let's wait until after 1.2.

However, Borja, if you don't mind, I'd really like to do this myself as I have this in my mind for quite a long time and already thought of this.

* Views like ThreadUI will have methods like "enter" and "leave" that will be called by the Navigation object. These methods will contain most of the code that is in the current MessageManager's onHashChange method.
* the Navigation object will handle the onHashChange event and will do all the hash changes themselves
* the Navigation object will keep a map of hashes/view objects/view logical name, and the other objects will only know the view logical name. Another possibility is to make the view objects have a property "name" that defines their logical name, the navigation object would autodiscover them. But I think that I prefer the first possiiblity.

Note: gnarf has a WIP for clock in https://github.com/gnarf/gaia/pull/1, even if I don't agree with everything it's worth having a similar implementation.
depending on bandwidth and priority this can be addressed in Oslo.
Whiteboard: [comms-triaged]
Taking, I did some preliminary work already.
Assignee: fbsc → felash
blocking-b2g: koi? → ---
Updated my code at https://github.com/julienw/gaia/compare/881469-navigation-object

still not ready for prime time yet.
Attached patch patch v1 (obsolete) — Splinter Review
Github PR at https://github.com/mozilla-b2g/gaia/pull/16377

This commit implements a Navigation abstraction to handle navigating between
views. It also introduces the concept of views.
---
 apps/sms/index.html                              |    3 +-
 apps/sms/js/activity_handler.js                  |   62 +-
 apps/sms/js/information.js                       |   16 +-
 apps/sms/js/message_manager.js                   |  277 +----
 apps/sms/js/navigation.js                        |  295 +++++
 apps/sms/js/recipients.js                        |    5 +-
 apps/sms/js/startup.js                           |    3 +-
 apps/sms/js/thread_list_ui.js                    |   38 +-
 apps/sms/js/thread_ui.js                         |  372 +++++--
 apps/sms/js/threads.js                           |   30 +-
 apps/sms/style/sms.css                           |    4 +-
 apps/sms/test/unit/activity_handler_test.js      |   67 +-
 apps/sms/test/unit/drafts_test.js                |    7 +-
 apps/sms/test/unit/information_test.js           |   57 +-
 apps/sms/test/unit/message_manager_test.js       |  563 +---------
 apps/sms/test/unit/mock_message_manager.js       |   15 +-
 apps/sms/test/unit/mock_navigation.js            |   12 +
 apps/sms/test/unit/navigation_test.js            |  380 +++++++
 apps/sms/test/unit/recipients_test.js            |   13 +-
 apps/sms/test/unit/sms_test.js                   |    7 +-
 apps/sms/test/unit/thread_list_ui_test.js        |   27 +-
 apps/sms/test/unit/thread_ui_integration_test.js |   17 +-
 apps/sms/test/unit/thread_ui_test.js             | 1266 ++++++++++++++--------
 apps/sms/test/unit/threads_test.js               |   38 +-
 24 files changed, 2000 insertions(+), 1574 deletions(-)
 create mode 100644 apps/sms/js/navigation.js
 create mode 100644 apps/sms/test/unit/mock_navigation.js
 create mode 100644 apps/sms/test/unit/navigation_test.js

So, here it is, at last!


I've "translated" all old-style tests to tests using a Navigation stub.

I've leveraged Promises to build a asynchronous-friendly API. I think we can make great use of Promises in other areas too.

In a close future, I'd like to separate the Composer and ThreadUI views in 2 separate objects (that could both live in the same js file, or not), because they don't share much logic in the end. Also I'd like to move some things from afterEnter to beforeEnter so that the user does not "see" the panel populating. But in the same time we want it to slide as soon as possible, so we'll have to think a little.

I also want to make the sliding behavior better, so that the GroupView will be able to slide too.

I also want to change hashes when we change views (something I left over in this first patch) so that we can bookmark views (needed for Haida). I already should handle correctly a hash at the start of the app, although if it doesn't work now, I'd rather fix it in a follow-up.

I also want to lazy load views.

I've left some other TODO in the code because I think we can make it better, but I want to land it so that we can build upon it and make it better with follow-up patches.

If we all agree on this, I'll file follow-up bugs for all this.

Asking feedback from all peers because we need more eyes on this important topic. I'd like to test a little more on my device before asking a "real" review.

Also, I see I need a rebase because of 2 commits that landed today, will rebase this tomorrow.
Attachment #8377245 - Flags: feedback?(waldron.rick)
Attachment #8377245 - Flags: feedback?(schung)
Attachment #8377245 - Flags: feedback?(borja.bugzilla)
Comment on attachment 8377245 [details] [diff] [review]
patch v1

I left a few questions, but on the whole this is really excellent. So much nasty code is gone. 

A little bike shedding: how do you feel about changing Navigation.currentPanel('foo') to Navigation.isCurrentPanel('foo')?
(In reply to Rick Waldron [:rwaldron] from comment #16)
> Comment on attachment 8377245 [details] [diff] [review]
> patch v1
> 
> I left a few questions, but on the whole this is really excellent. So much
> nasty code is gone. 
> 
> A little bike shedding: how do you feel about changing
> Navigation.currentPanel('foo') to Navigation.isCurrentPanel('foo')?

So you mean, basically, having a "getCurrentPanel()" and a "isCurrentPanel(panel)" ?

Currently I don't use the "getCurrentPanel" form though, so maybe it makes sense to expose only the "interrogation form" only.

(also, for other feedbackers, note that I already identified regressions on the device, so please just comment the general code for now :) )
I just left few comments and I'll have a deep look later since I just got some torako tasks now... :/
Anyway, it looks decent with hash related logic clean up and introduce promise in the navigator. This patch is awesome!
Blocks: 974763
Target Milestone: --- → 1.4 S6 (25apr)
Blocks: 833762
Attachment #8377245 - Flags: feedback?(waldron.rick)
Attachment #8377245 - Flags: feedback?(schung)
Attachment #8377245 - Flags: feedback?(borja.bugzilla)
Just pushed a rebased version, I didn't test the result yet.
I also implemented some requested follow-ups, and some fixes.
Pushed a new version, I still have a wrong/weird behavior if we switch panels while edit mode is active, but otherwise it looks good.
Blocks: sms-sprint-1
Target Milestone: 1.4 S6 (25apr) → 2.0 S2 (23may)
Whiteboard: [comms-triaged] → [comms-triaged][p=3]
Whiteboard: [comms-triaged][p=3] → [p=3][comms-triaged]
Blocks: 1009541
Blocks: 1009545
Blocks: 1009568
Blocks: 1010216
Blocks: 1010223
Blocks: 1010318
Attached file github PR
Attachment #8377245 - Attachment is obsolete: true
Blocks: 995035
Blocks: 1011085
Blocks: 1011089
Comment on attachment 8423236 [details] [review]
github PR

Steve, Oleg, I think this is now in a good state to have a first review.

See comment 15 for all the things I decided to leave for follow-up bugs. I think I created all of them already and made them depend on this bug. I also left comments in the code with references to the bugs numbers.

Have fun!
Attachment #8423236 - Flags: review?(schung)
Attachment #8423236 - Flags: review?(azasypkin)
Blocks: 984920
Leave some comments and question in the github, but most of all is good and I can't find other issue not in the follow-up bugs. I'll still keep the review flag since I haven't looked through all the unit test changes and you might finish the 1st round shortly ;)
(In reply to Steve Chung [:steveck] from comment #23)
> Leave some comments and question in the github, but most of all is good and
> I can't find other issue not in the follow-up bugs. I'll still keep the
> review flag since I haven't looked through all the unit test changes and you
> might finish the 1st round shortly ;)

I won't push another round before the start of your day tomorrow, so don't worry.
(and you should really go to sleep now ;) ).
blocking-b2g: --- → backlog
feature-b2g: --- → 2.0
Comment on attachment 8423236 [details] [review]
github PR

Left several comments and questions at GitHub. Overall it looks good and I'm happy that you've removed a lot of nasty code and made app ready for further improvements! I haven't tested it in on real device much, but will dogfood it during this week.

Please, ask for review again whenever you're ready :)

Thanks!
Attachment #8423236 - Flags: review?(azasypkin)
Comment on attachment 8423236 [details] [review]
github PR

Ready for another review !

I fixed all nits in a second commit. Please test and see if there is any strange behavior !
Attachment #8423236 - Flags: review?(azasypkin)
Flags: needinfo?(schung)
Comment on attachment 8423236 [details] [review]
github PR

I left some questions on github, but I found one more unexpected behavior here:

1) Enter a thread and forward a message,
2) Exit the forward message and save to draft
3) Back to the list view, you could see the draft is saved in thread opened in (1), but it should belong to no recipient.
4) Enter the thread again and leave.

Now if you enter the new composer, the recipients and mesage in (4) will show up
, and draft icon in list view will not disappear. Julien, could you please investigate this issue? Sorry I haven't look into it right now but this issue didn't exist before this patch...
Attachment #8423236 - Flags: review?(schung)
Flags: needinfo?(schung)
Thanks !

I have bug 1010216 to revisit how drafts are handled after this patch lands, but I don't want to regress, so I'll look at this right now.
There is the same issue when we send a message to a number extracted and linked from a SMS message.
Comment on attachment 8423236 [details] [review]
github PR

Fixed the nits + the issue that Steve reported. We were not properly reseting Threads.currentId when going from thread ui to composer (we were only reseting it from thread ui to thread list).

Now I'm additionnally resetting it in beforeEnter for Composer (symetric because we set it in beforeEnter for a thread).

3rd review cycle !
Attachment #8423236 - Flags: review?(felash)
Attachment #8423236 - Flags: review?(felash) → review?(schung)
Comment on attachment 8423236 [details] [review]
github PR

It looks good code wise and on real device! Just left a few minor code style nits at GitHub. Noticed and filed a bug 1015755 while testing it on device, but it's old one and not related to this patch :)

Thanks!
Attachment #8423236 - Flags: review?(azasypkin) → review+
Comment on attachment 8423236 [details] [review]
github PR

It works perfact! r=me with nits addressed and let's focus on the follow-up issues. Thanks for the awesome jobs :)
Attachment #8423236 - Flags: review?(schung) → review+
Fixed latest nits, squashed commits and rebased. Now waiting for a latest travis round.
Travis is green !

master: cd56ecb15b8c666973379c76cb6b78da320fc498
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 984920
Depends on: 1022755
Depends on: 1026575
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.