1065279 - [settings] refactor Hotspot panel with AMD pattern

Status: RESOLVED FIXED

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

Description

[[e-mail]]

refactor Hotspot panel with AMD pattern according to https://github.com/mozilla-b2g/gaia/tree/master/apps/settings

Comment 1

[Manuel Casas Barrado [:mancas]]

Attachment: Hotspot panel refactor

Hey Arthur! This is my first refactor so please take a look at the commit.

Thanks!

Comment 2

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8489327 [details] [review]
Hotspot panel refactor

Manuel, thanks for the patch!

In addition to covert the original scripts to AMD modules, we should also refactor the code and make it more readable and testable. It can be achieved by breaking the script into smaller reusable modules and separating UI and model.

Note that we should also utilize the basic functions for example onBeforeShow or onShow function instead of the panel ready event (which is depreciated). Details please refer to https://github.com/mozilla-b2g/gaia/tree/master/apps/settings.

Comment 3

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8489327 [details] [review]
Hotspot panel refactor

Hey Arthur!

Take a look at the PR when you have time.

Thanks!

Comment 4

[Manuel Casas Barrado [:mancas]]

Attachment: Hotspot panel refactor

Comment 5

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8498770 [details] [review]
Hotspot panel refactor

Thanks for the patch! We are on the right track.

I have some comments regarding the module design of the feature. One of the goal of the refactoring plan is to split UI and data (business logic), which makes writing unit tests more easily and also helps on readability and maintainability. We should always keep the goal in mind when designing modules.

The feature comprises two panels:
1. Toggles for enabling/disabling two types of internet sharing. There is also UI for displaying the hotspot settings.
2. UI for manipulating the hotspot settings.

It is easy to identify that the shared data across the panels is the hotspot settings, so it would make sense to have a module representing hotspot settings. The module should store the hotspot settings and provide methods for manipulating the settings.

It is encouraged to use Observable[1] in the module. By doing this we can bind a property to UI elements. Then the UI gets updated automatically when the property changes. It is useful on displaying name and security of the hotspot settings and keeping them updated.

You could refer to this file[1] for details on implementing on such modules.

[1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/modules/mvvm/observable.js
[2]: https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/panels/display/wallpaper.js

Let me know if you encounter any problems!

Comment 6

[Manuel Casas Barrado [:mancas]]

Hey Arthur!

At first, thank for your time and patience.

I've updated the PR, I believe we already have the two panels |hotspot| and |hotspot_wifi_settings|. Besides, I've added the module you suggested in the previous comment. So, now, the security and the name of the hotspot are updated automatically when the setting changes.

However, I have some doubts about the HotspotContext file that I made. What do you think about it? Will we use it to manage the hotspot settings and throw events to update the UI?

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

The new hotspot settings is looking good!

It looks like both panels create its own instance of hotspot settings module doing the same things. We could create only one instance in |hotspot| and pass it to |hotspot_wifi_settings| for editing. You can use `SettingsService.navigate` to navigate to a panel with options, and implementation detail please refer to here[1]. For the hotspot settings module itself, it should define more specific methods for manipulating the setting instead of generic moz settings accessors.

HotspotContext is only used by |hotspot_wifi_settings|, so I would suggest to rename it to |hotspot| and place to /panels/hotspot/ expressing that it serves the |hotspot| panel. It will still manage UI related to togging wifi and usb tethering, but again it would be better to have specific methods instead of `setMozSetting` or `getMozSetting`.

[1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/panels/apn_list/panel.js#L65

Comment 8

[Manuel Casas Barrado [:mancas]]

I'm having some problems with the 'Ok' button and the 'show password' checkbox. They are reset before the panel is shown, however, it just work the first time the user enter in the panel. After that, the 'ok' button is always enabled and the checkbox remembers the state.

I've tried to reset them when the panel is hidden but didn't work. Any ideas?

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

The state of ok button depends on the content of the hotspot settings to be edited. The ok button should remain enabled if the password is valid.

As for the checkbox, replacing `elements.passwordInput.checked = false;` with `elements.showPassword.checked = false;` should fix the problem.

Comment 10

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8498770 [details] [review]
Hotspot panel refactor

Ok Arthur, I think it's time to take a look at the whole code when you want and please give me feedback before start making unit tests.

Thanks!

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8498770 [details] [review]
Hotspot panel refactor

f=me! You revised the patch so fast that I couldn't catch up. :p

The code already looks good and there should be no significant change required.
I haven't reviewed the tests. Will check the test when you request a review. Thanks!

Comment 12

[Manuel Casas Barrado [:mancas]]

Arthur, I've some problems with the unit tests. I'm trying to load the modules but I get this error:

Error: TypeError: navigator.addIdleObserver is not a function (app://settings.gaiamobile.org/js/modules/panel_cache.js?bust=1413364463930:41)
    at onerror (app://settings.gaiamobile.org/common/vendor/mocha/mocha.js:4959:10)

Could you take a look at this, please?

Thanks!

Comment 13

[Arthur Chen [:arthurcc][inactive after 6/30]]

Please make sure that the paths of the modules you used in the tests are all correct. For example, `modules/hotspot_context` no longer exists but is still used in the tests.

Comment 14

[Manuel Casas Barrado [:mancas]]

Sorry, I forgot to delete that file. The problem is happening in |panel_test.js|

Thanks

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

`navigator.addIdleObserver` is not available on desktop firefox, so we should use a mock function when necessary. When writing unit tests for a module, we should determine which dependent modules can be replaced by mock objects for ease of testing.

Take |panels/hotspot/panel.js| for example, it requires `SettingsService` because it uses `SettingsService.navigate`. From the aspect of unit testing, we don't care about how `SettingsService.navigate` works but only whether it is called with correct parameters. Therefore, we can use a mock of `SettingsService` with `navigate` stubbed.

And there is one more thing, which is since we've already separated the business logic to `hotspot` and `hotspot_settings`, you could then notice that there are only UI binding logic left in `panel.js`. I'm fine with leaving it without unit tests because it is relatively simple and less error-prone. Instead, we should focus on writing robust unit tests for the modules containing business logic because this kind of modules is the foundation of supporting the UI and is not going to (and should not) be changed frequently when there are UI spec changes.

Comment 16

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8498770 [details] [review]
Hotspot panel refactor

Hey Arthur, thanks for the explanation!

Take a look at the PR when you want.

Thanks again!

Comment 17

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8498770 [details] [review]
Hotspot panel refactor

We are almost there. Left some comment in github, please check them, thanks!

Comment 18

[Manuel Casas Barrado [:mancas]]

Could you clarify me some comments in github, please?

Thanks!

Comment 19

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8498770 [details] [review]
Hotspot panel refactor

I've updated the patch. Please review the PR when you want.

Thanks for your patience

Comment 20

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8498770 [details] [review]
Hotspot panel refactor

There seems some errors in path so I couldn't load the panel. And the hotspot_test.js fails. I also left some comments in github, please check, thanks.

Comment 21

[Manuel Casas Barrado [:mancas]]

Attachment: logcat

Arthur, I have some problems with the panels. First, an unexpected JS error is thrown when trying to cancel the |incompatibleSettingsDialog|. Please see the attached log.

The |hotspot_test| is failing because the object |hotspot| is not correctly instantiated, what I mean is that the object doesn't have own methods such as |_hotspotSettingChange|

Could you help me with this problems?

Comment 22

[Arthur Chen [:arthurcc][inactive after 6/30]]

I believe the first problem was caused by the second one. A module cannot be instantiated correctly if it cannot locate the modules it requires. Please double check the paths and make sure they are all correct.

Comment 23

[Manuel Casas Barrado [:mancas]]

I've solved the unexpected JS error. The dialog buttons were submitting the form.

Review the patch when you have enough time.
Thanks for all

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8498770 [details] [review]
Hotspot panel refactor

The code regarding add/remove listeners still incorrect, please check my comments, thanks.

Comment 25

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8498770 [details] [review]
Hotspot panel refactor

I've checked the listeners and they're removed properly when the panel is hidden.

Take a look when you have time.

Thanks Arthur

Comment 26

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8498770 [details] [review]
Hotspot panel refactor

Looks great! r=me, thanks!

Comment 27

[Arthur Chen [:arthurcc][inactive after 6/30]]

master: ac5331fba28df093621c606afdbe74f33c7c48c0