ABP/Android preferences UI

[Felix Dahlke, 2012-09-18 15:32:49]

That was quite a lot to review, but I tried to be thorough. I'm doing a lot of
nit picking there, but that 's mostly occasions where something made it a bit
more difficult for me to understand the code, and readability is what I'm
asserting here.

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
File src/org/adblockplus/android/AboutDialog.java (right):

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AboutDialog.java:20: private static Context mContext
= null;
Shouldn't class variables be prefixed with "s" when this convention is used? I
know it like that, so I was a bit surprised that mContext is not an instance
variable. I don't really mind as long as it's consistent though.

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AboutDialog.java:78: {
Empty catch blocks are in most cases not a good thing. We should either log
something or throw another exception here.

Unless it really can be safely ignored, in which case I'd like a reassuring
comment :)

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
File src/org/adblockplus/android/ConfigurationActivity.java (right):

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/ConfigurationActivity.java:31: final Intent intent =
new Intent(Intent.ACTION_VIEW, uri);
Why make intent final here?

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
File src/org/adblockplus/android/Preferences.java (right):

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:82:
AdblockPlus.getApplication().startEngine();
Since you're using the result of getApplication() twice, it would be a good idea
to assign it to a temporary variable and call getApplication() just once.
Probably won't be an issue, but you can't be sure that the result won't be
different unless you go and inspect getApplication().

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:87: public void onResume()
I'd love it if you could move some of this method's code into helper methods.
It's a bit hard for me to wrap my head around long methods.

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:96: String[] entries = new
String[subscriptions.size()];
Like above, I'd appreciate it if you call subscriptions.size() just once.

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:168: else if (! enabled &&
firstRun)
I think "!enabled" read better than "! enabled". I know, nit picking :)

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:196:
AdblockPlus.getApplication().stopEngine(true);
Double getApplication() again, see above.

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:271: in = null;
You don't need to set in to null, it's going out of scope for the next iteration
of the for loop. Same with out below.

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:357: public void onReceive(final
Context context, Intent intent)
Another somewhat large method, can you split it up a bit?

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:367:
showConfigurationMsg(getString(R.string.msg_configuration,
extra.getInt("port")));
This string is used in multiple places, how about a constant?

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:403: if (id > 0)
How about using ?: here? Would safe three lines. I don't mind if you'd rather
not :)

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:409: builder.append(": ");
I think 8 nesting levels are too much. Can you refactor this?

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
File src/org/adblockplus/android/RefreshableListPreference.java (right):

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/RefreshableListPreference.java:26: final ViewGroup
widgetFrameView = ((ViewGroup) view.findViewById(android.R.id.widget_frame));
Why is widgetFrameView final?

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/RefreshableListPreference.java:31: final float
mDensity = getContext().getResources().getDisplayMetrics().density;
Why prefix mDensity with an "m"? It's a local variable

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
File src/org/adblockplus/android/SubscriptionParser.java (right):

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/SubscriptionParser.java:20: private Subscription
subscription;
Could you give this a more descriptive name, like "currentSubscription" (if
that's what it is)?

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
File src/org/adblockplus/android/SummarizedPreferences.java (right):

http://codereview.adblockplus.org/8363097/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/SummarizedPreferences.java:18: // initialize list
summaries
I don't think this comment is necessary.