881469 - [MMS] Implement Navigation object

Status: RESOLVED FIXED

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

Description

[Mike Pennisi [:jugglinmike]]

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.

Comment 1

[Julien Wajsberg [:julienw]]

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.

Comment 2

[Mike Pennisi [:jugglinmike]]

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?

Comment 3

[Julien Wajsberg [:julienw]]

Nope, that doesn't exist and I'm not aware of any plan about doing this.

Comment 4

[Alex Keybl [:akeybl]]

Please renominate when we have a real world user scenario.

Comment 5

[Julien Wajsberg [:julienw]]

I think we'll do this once we start the 1.2 development cycle... unless this bites us too much.

Comment 6

[Borja Salguero [:borjasalguero]]

Im gonna take this as well after talking with Julien. It's gonna be a part of the work started here 880115.

Comment 7

[Julien Wajsberg [:julienw]]

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.

Comment 8

[Mike Pennisi [:jugglinmike]]

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?

Comment 9

[Julien Wajsberg [:julienw]]

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 ?

Comment 10

[Borja Salguero [:borjasalguero]]

This bug could be a great one for landing during Oslo WW, due to we could discuss together the details about it.

Comment 11

[Julien Wajsberg [:julienw]]

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.

Comment 12

[Wilfred Mathanaraj [:WDM]]

depending on bandwidth and priority this can be addressed in Oslo.

Comment 13

[Julien Wajsberg [:julienw]]

Taking, I did some preliminary work already.

Comment 14

[Julien Wajsberg [:julienw]]

Updated my code at https://github.com/julienw/gaia/compare/881469-navigation-object

still not ready for prime time yet.

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: patch v1

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.

Comment 16

[Rick Waldron [:rwaldron]]

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')?

Comment 17

[Julien Wajsberg [:julienw]]

(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 :) )

Comment 18

[Steve Chung [:steveck]]

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!

Comment 19

[Julien Wajsberg [:julienw]]

Just pushed a rebased version, I didn't test the result yet.
I also implemented some requested follow-ups, and some fixes.

Comment 20

[Julien Wajsberg [:julienw]]

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.

Comment 21

[Julien Wajsberg [:julienw]]

Attachment: github PR

Comment 22

[Julien Wajsberg [:julienw]]

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!

Comment 23

[Steve Chung [:steveck]]

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 ;)

Comment 24

[Julien Wajsberg [:julienw]]

(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 ;) ).

Comment 25

[Oleg Zasypkin [:azasypkin]]

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!

Comment 26

[Julien Wajsberg [:julienw]]

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 !

Comment 27

[Steve Chung [:steveck]]

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...

Comment 28

[Julien Wajsberg [:julienw]]

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.

Comment 29

[Julien Wajsberg [:julienw]]

There is the same issue when we send a message to a number extracted and linked from a SMS message.

Comment 30

[Julien Wajsberg [:julienw]]

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 !

Comment 31

[Oleg Zasypkin [:azasypkin]]

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!

Comment 32

[Steve Chung [:steveck]]

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 :)

Comment 33

[Julien Wajsberg [:julienw]]

Fixed latest nits, squashed commits and rebased. Now waiting for a latest travis round.

Comment 34

[Julien Wajsberg [:julienw]]

Travis is green !

master: cd56ecb15b8c666973379c76cb6b78da320fc498