refactor Hotspot panel with AMD pattern according to https://github.com/mozilla-b2g/gaia/tree/master/apps/settings
Created attachment 8489327 [details] [review] Hotspot panel refactor Hey Arthur! This is my first refactor so please take a look at the commit. Thanks!
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 on attachment 8489327 [details] [review] Hotspot panel refactor Hey Arthur! Take a look at the PR when you have time. Thanks!
Created attachment 8498770 [details] [review] Hotspot panel refactor
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 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 for details on implementing on such modules. : https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/modules/mvvm/observable.js : https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/panels/display/wallpaper.js Let me know if you encounter any problems!
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?
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. 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`. : https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/panels/apn_list/panel.js#L65
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?
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 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!
Attachment #8498770 - Flags: feedback?(arthur.chen)
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!
Attachment #8498770 - Flags: feedback?(arthur.chen) → feedback+
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!
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.
Sorry, I forgot to delete that file. The problem is happening in |panel_test.js| Thanks
`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 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 on attachment 8498770 [details] [review] Hotspot panel refactor We are almost there. Left some comment in github, please check them, thanks!
Could you clarify me some comments in github, please? Thanks!
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 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.
Created attachment 8511909 [details] 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?
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.
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 on attachment 8498770 [details] [review] Hotspot panel refactor The code regarding add/remove listeners still incorrect, please check my comments, thanks.
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 on attachment 8498770 [details] [review] Hotspot panel refactor Looks great! r=me, thanks!
Attachment #8498770 - Flags: review?(arthur.chen) → review+
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.