[Replicant] [PATCH] Hide information leaking phone number lookup settings

Denis 'GNUtoo' Carikli GNUtoo at cyberdimension.org
Fri Oct 11 12:50:53 UTC 2019


On Thu, 10 Oct 2019 22:02:53 +0300
Joonas Kylmälä <joonas.kylmala at iki.fi> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.osuosl.org/pipermail/replicant/attachments/20191011/212715d8/attachment.asc>


More information about the Replicant mailing list