1115644 - Implement RootPanel and standardize panel interface for KeyboardSettingsApp

Status: RESOLVED FIXED

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

Description

[John Lu [:mnjul][Please NI or I'll miss]]

In bug 1102831, I implemented two extra panels/dialogs in KBSettingsApp and implemented a PanelController there. However, the "root" panel was still controlled by the old codes, resulting in exception logics and inconsistency. 

Let's implement the root panel and straighten things.

Comment 1

[John Lu [:mnjul][Please NI or I'll miss]]

Alright, I'm on this.

Comment 2

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Patch (PR @ GitHub)

Let's check if integration tests don't fail (they shouldn't)

Comment 3

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8548590 [details] [review]
Patch (PR @ GitHub)

Tim,

Please check if this is going under the correct direction; especially if there’s any chance it might conflict with your downloadable dictionary work.

Things to notice:

1. To accommodate the life cycle of root panel, The start/stop functions of SettingsView (and those using it) have been split into: init, start, stop, uninit. The rationale is that init and uninit shall only be called once throughout an object’s lifetime, while start and stop may be called multiple times, and are used when we transition-in and transition-out to/from root panel, respectively. The naming can be adjusted, for sure. Yet I’m less certain that SettingsView and SettingsPromiseManager are robust enough to be agnostic to the change, so you might want to check.

2. In my codes the knowledge of existence of UserDictionary functionality has been again decentralized: KBSettingsApp has to know whether the class exists and RootPanel has to know whether the menu item exists over there.

Thanks!

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8548590 [details] [review]
Patch (PR @ GitHub)

Thanks for the patch and the comment here.

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #3)
> Comment on attachment 8548590 [details] [review]
> WIP Patch (PR @ GitHub)
> 
> Tim,
> 
> Please check if this is going under the correct direction; especially if
> there’s any chance it might conflict with your downloadable dictionary work.
> 
> Things to notice:
> 
> 1. To accommodate the life cycle of root panel, The start/stop functions of
> SettingsView (and those using it) have been split into: init, start, stop,
> uninit. The rationale is that init and uninit shall only be called once
> throughout an object’s lifetime, while start and stop may be called multiple
> times, and are used when we transition-in and transition-out to/from root
> panel, respectively. The naming can be adjusted, for sure. 

What's the difference between start/start and show/hide function then?

This makes the Panel instances comes with 8(!) methods:

init/uninit/start/stop/beforeShow/afterHide/show/hide

I am not convinced we need 8 (or, confused on why we need 8). We probably need 6 at most.

Please document what these methods do and make all panels inherits from a PanelView/PanelBase class.

(I also wonder if we want to create a SettingsViewBase class as well.)

> Yet I’m less
> certain that SettingsView and SettingsPromiseManager are robust enough to be
> agnostic to the change, so you might want to check.

I can't say that for sure as well. stop() the observation of settings will probably make the settings & UI out-of-sync -- and you would have to re-sync the settings after start(). I don't think we really need to do that, at least not in this patch.

> 2. In my codes the knowledge of existence of UserDictionary functionality
> has been again decentralized: KBSettingsApp has to know whether the class
> exists and RootPanel has to know whether the menu item exists over there.
> 
> Thanks!

A general way to do this would be start using hashes to navigate between panels, just like Settings app, but that might result some undeterministic viewport shifting behavior, so I don't really care if you want to keep the current setup.


Comments other than above:

1. RootPanel should probably rename to GeneralPanel, and initialized by PanelController. I don't see KeyboardSettingsApp touches the panel directly anymore, with this patch.
2. PanelController should not be in the business of touching the RootPanel#_container property directly -- if it needs to do that we should probably remove the underline in the naming of this property.
3. Improvement: Get the *Panel APIs symmetrical and make PanelController find out which to init as root with a config property (e.g. `PanelController#ROOT_PANEL_CLASS = GeneralPanel;`), and we will get ourselves a setup that allow us to load the page to any panel directly, for free.

PS Since the UI of bug 1029951 live on the general panel, your panel view classes shouldn't affect my patch a lot (except the function naming). So please continue working on this and I will rebase on top of the patch accordingly.

Comment 5

[John Lu [:mnjul][Please NI or I'll miss]]

Thanks, Tim.

Offline discussed with Tim and we agreed upon an architecture plan largely based on comment 4. In this bug I plan to:
1. Implement the base class of a panel, document its API.
2. Not care about the suspend-ability and resume-ability about SettingsPromiseManager, and subsequently, SettingsView and those users of SettingsView. So we don't have start/init/uninit/stop differentiation.
3. Canonically rename the idea of init/uninit to start/stop to match the vocabulary in other KeyboardApp components.
  
Will file a follow-up bug to keep track of things descoped from this bug.

Comment 6

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8548590 [details] [review]
Patch (PR @ GitHub)

Tim,

Please check out the revised patch. Noticeable changes:

1. I decided to start() the panels in panelController's start() and not wait until beforeShow(). I believe the more symmetry lowers confusion.

2. Both ViewBase and PanelBase have been added. ViewBase is something empty -- it just dictates the interface (start/stop/beforeShow/show/beforeHide/hide) and provides the "do nothing by default" mechanism for subclasses. Subclasses are "Views", and PanelBase: PanelBase is more specialized: it can contain some Views and will take care of the (possible) asynchronousness of the views when it needs to propagate show/beforeShow/hide/beforeHide hooks.

This is where we might be over-designing as only GeneralPanel contains Views and UserDictionary panels/dialogs don't, and they don't use the PanelBase class's constructs. Are the base class's constructs good/futureproof enough to be placed here in this bug?

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8548590 [details] [review]
Patch (PR @ GitHub)

Looks good other than what we have discussed.

Comment 8

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8548590 [details] [review]
Patch (PR @ GitHub)

Alright, patch amended, tests added/amended. Please check this out. Thanks!

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8548590 [details] [review]
Patch (PR @ GitHub)

Nice!

Comment 10

[John Lu [:mnjul][Please NI or I'll miss]]

Master: https://github.com/mozilla-b2g/gaia/commit/dbee4c43f4600307475faeff9517397a3cd0db31