[tor-bugs] #28329 [Applications/Tor Browser]: Design TBA+Orbot configuration UI/UX
Tor Bug Tracker & Wiki
blackhole at torproject.org
Thu Mar 7 11:57:38 UTC 2019
#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
More information about the tor-bugs
mailing list