#28329: Design TBA+Orbot configuration UI/UX -------------------------------------------------+------------------------- Reporter: sysrqb | Owner: tbb- | team Type: enhancement | Status: | needs_revision Priority: Very High | Milestone: Component: Applications/Tor Browser | Version: Severity: Normal | Resolution: Keywords: tbb-mobile, ux-team, TBA-a3, | Actual Points: TorBrowserTeam201902 | Parent ID: | Points: Reviewer: | Sponsor: | Sponsor8 -------------------------------------------------+-------------------------
Comment (by gk): Here comes the detailed review adding to the things mentioned in my previous comments. Overall: really nice work! The biggest issue I have is dealing with our bridge pref handling. I feel we should try avoid custom parsing code (aka TorBridgeList.java). It's less error prone and probably easier to just write a script that converts the .js file we have right now for desktop into whatever is easiest to deal with on mobile during our build. Anyway, here are my comments: I looked at the code commit-wise and then for b2c12e4881e3aacf192d3cf623028246f40ab3c4 I wrote comments down per file and for `TorPreferences.java` per class and method. This review is based on code in `28329_v6`. dc4d883cefa60af9caa3894360e7f07992174fa1 - not okay {{{ + <color name="tor_input_background">#B0B0B0</color> }}} declared, but where is this needed/used? 76e19a2eaa415a5465b4f103232b862362dec515 - okay 183b5baae0849aa0263c47b6df4306110e978070 - okay b2c12e4881e3aacf192d3cf623028246f40ab3c4 general: often `if (null != $foo)` and `if ($foo != null)` mixed, please stick to the latter. That's the overwhelming pattern when doing `null` checks in the mobile code (although I am not sure why there are small exceptions). Is `preference_tor_network_bridges_enabled.xml` copied over from somewhere? If so, please add a hint from where. (I got curious as it's the only file that contains an Apache license and it mentions a `PreferenceActivity` (you use `PreferenceFragment`, though). There are some .xml files that contain MPL 2.0 license headers and some not, please make the behavior consistent. (I guess this means adding the respective license where it is issing). `TorAnimationContainer.java` {{{ public void hide() { visible = false; }}} superfluous newline `this.onFinishListener = listener;` Is `this` here needed? You don't have it in `hide()`? `TorBootstrapLogger.java` - not okay Missing license header? `TorBootstrapLogPanel.java` - okay `TorBootstrapPanel.java` - not okay s/persistant/persistent/ {{{ // menu again. If the don't solve the problem, then we'll disable it // again. }}} "If they don't"? {{{ if (null != spinningOnion) { // Stop spinning. This is null if we didn't // previously call startBootstrapping. spinningOnion.stop(); // Make the still image 0% transparent, but only }}} How can this be `null` after you checked for it not being `null`? Should the comment go above the `if`-block? "Make the still image"? Something seems to be missing in that comment. `TorLogEventListener.java` - okay `TorBootstrapPagerConfig.java` - not okay Indentation is off by 1 for `getDefaultConnectPanel()` and `getDefaultBootstrapPanel()`. `TorBootstrapPager.java` - okay `TorBridgeList.java` - not okay What about distributing the prefs in `torbrowser/assets/distribution/preferences.json` and using e.g. `DistroSharedPrefsImport` instead of doing our own pref import/parsing? Or some other more Android-y way of dealing with that that does not involve our custom prefs parse logic... `TorPreferences.java` {{{ + * This class (PreferenceActivity) }}} You meant `(TorPreferences)`? {{{ + * pref_bridges_enabled=null }}} `false` instead of `null` The line length in the big comment above the class is different which is confusing {{{ + * always match, and pref_bridges_list must either match }}} superfluous whitespace at the end of the line TorPreferences ::onCreate() (okay) ::isValidFragment() (okay) TorNetworkPreferenceFragment ::onCreate() (okay) ::walkTreeView() <- just for printing debug information? Do we need that at all the places currently using it? ::getListView() {{{ if (!(view instanceof ViewGroup)) { return null; } if (view == null) { return null; } }}} better: {{{ if (view == null || !(view instanceof ViewGroup)) { return null; } }}} ::getBridges() (okay, but see my general comment above) ::setBridges() {{{ // Set Orbot's preference Prefs.setBridgesList(bridgeLine); if (!editor.commit()) { return false; } }}} Swap both so that we are sure writing the pref succeeded and set the `Orbot` prefs only afterwards to be not out of sync? ::disableBridgesEnabledPreference() <- why not just "disableBridges()"? {{{ // Clear the saved prefs (it's okay we're using a different // SharedPreference.Editor here, they modify the same backend }}} closing parenthesis is missing and "." ::setTitle() (okay) TorNetworkBridgeEnabledPreference ::onCreate() {{{ // Only launch Fragment is we're enabling bridges. }}} s/is/if/ ::onViewCreated() {{{ // when Preferences the layout is created across multiple }}} Something is missing here. s/the Change buttom/the Change button/g ::setBridgeChangeOnClickHandler() - okay ::isBridgeProvided() {{{ // If the bridgesEnabled switch is off }}} s/bridgesEnabled/bridgeEnabled/, given that you are using `bridgeEnabled` in the following code, no? I am debating whether it would clearer to do as/bridgeEnabled/bridgesEnabled/g, given that this belongs to `PREFS_BRIDGES_ENABLED` which is plural. There is a similar tension between `bridgeType`/`bridgesType` for `PREFS_BRIDGES_TYPE`. TorNetworkBridgeSelectPreference ::onCreate() - okay ::onViewCreated() - okay ::addBridgeTypeRadioGroupOnCheckedHandler() {{{ // This onl operates on }}} s/onl/only/ ::getBridgeTypeRadioGroup() - okay ::getBridgeTypes() - okay ::getViewSeparator() - not needed right now (together with the two lager TODO blocks) ::populateList() {{{ // The preferences are adding }}} s/adding/added/ ::addBridgeTypeRadioButtons() - okay TorNetworkBridgeProvidePreference ::onCreate() - okay ::setBridgeProvideText() -- okay ::onViewCreated() -- okay TorBridgeProvideSaverOnClickListener ::TorBridgeProvideSaverOnClickListener() - okay ::onClick() - okay -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/28329#comment:40> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs