On Thu, 10 Oct 2019 22:02:53 +0300 Joonas Kylmälä <[email protected]> wrote:
> Hi, Hi, > If it makes it any more clear I can put it for v2 like this: > > Hide information leaking phone number lookup settings > > The phone number lookup service leaks private information so hide the > option to enable it. The lookup service is by default disabled so this > guarantees no information gets leaked if the user would accidentally > enable the service. This looks ok for me. > >> LookupSettingsFragment.class.getName(); > >> - target.add(lookupSettingsHeader); > >> + // target.add(lookupSettingsHeader); > > Here it would be better to instead remove completely the line, or > > if you want to leave something, add a text comment instead that > > explains what was removed and why. > > > > Note that I didn't have time to look into the broader context of > > that patch yet, so I don't know if lookupSettingsHeader should be > > completely removed or not. > > I will remove it completely in v2. Thanks for the review! I've took the time to look at the broader context and I would suggest to remove the full code block instead of just the last line: > final Header lookupSettingsHeader = new Header(); > lookupSettingsHeader.titleRes = R.string.lookup_settings_label; > lookupSettingsHeader.summaryRes = > R.string.lookup_settings_description; lookupSettingsHeader.fragment = > LookupSettingsFragment.class.getName(); > target.add(lookupSettingsHeader); This way, when reading the code, everything will be consistent and much more clear for the reader. Denis.
pgpr1smCPR1Dh.pgp
Description: OpenPGP digital signature
_______________________________________________ Replicant mailing list [email protected] https://lists.osuosl.org/mailman/listinfo/replicant
