ABP/Android UI

ABP/Android UI

[Wladimir Palant, 2012-10-05 10:14:08]

For reference: the original review for this code was
http://codereview.adblockplus.org/8363097/

[Felix Dahlke, 2012-10-09 14:27:29]

Done reviewing. I found a couple of nuts to pick, but no bugs :)

On a general note: I think we'll want two space indents for the XML as well.

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

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AboutDialog.java:20: private static Context mContext
= null;
What's with the "m"? The rest of the code base doesn't use a specific prefix for
denoting member variables.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AboutDialog.java:83: {
I don't think we can be sure that reading the files works unconditional. I'm in
favour of a RuntimeException and/or a log message here.

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

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:36: private final static
String TAG = "AdvancedPreferences";
How about "static final" instead of "final static"? That'd be consistent with
the SUN/Oracle conventions and the line below.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:61:
prefUpdate.setOnPreferenceClickListener(new OnPreferenceClickListener() {
How about putting this opening brace on its own line?

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:71:
prefConfiguration.setOnPreferenceClickListener(new OnPreferenceClickListener() {
Same as above, brace should be on its own line.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:116: {
You know I don't like long methods, this is one :) I don't insist on this being
split up, but it would be easier for me to understand that way.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:118: switch (id)
Why use a switch case if there's only one case handled? I'd prefer an if.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:123: try
The same code for getting the package info is in AboutDialog. Maybe this
warrants a utility method that ignores the exception? In this case that would
simplify the following lines to:

PackageInfo pi = Utils.getPackageInfo(this);
versionCode = pi.versionCode;

But if it becomes utility code, we can't really ignore the exception safely
anymore, we should throw a RuntimeException or log an error.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:144: if
(Build.VERSION.SDK_INT >= 12) // Honeycomb 3.1
I think this should be a constant, since it's also used above. Or better yet,
set a boolean field in the constructor.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:167:
messageText.setOnClickListener(new View.OnClickListener() {
Brace on a new line please.

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

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:41: private final static String TAG
= "Preferences";
As in AdvancedPreferences, "static final"?

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:77: copyAssets();
Why can this getPackageInfo fail while those in AboutDialog/AdvancedPreferences
can't?

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:91: public void onResume()
Another long method, same comment as for AdvancedPreferences.onCreateDialog().

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:132:
subscriptionList.setOnRefreshClickListener(new View.OnClickListener() {
Brace on its own line please.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:155: (new Thread() {
Brace on its own line please.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:172: enabled = false;
enabled isn't read past this point, Setting it to false here is not necessary

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:270: Log.e(TAG, e.getMessage());
Why not pass e as the third parameter, like below?

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:274: InputStream in = null;
in and out can be declared in the try block, they are not referenced outside it.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:288: in = null;
You don't need to set in/out to null, they aren't used anymore and will go out
of scope very soon.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:307: final Intent intent = new
Intent(Intent.ACTION_VIEW, uri);
Why make intent final?

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:322: showAbout = false;
Naming nit pick: This sounds imperative, how about "aboutShown"?

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:338: else if (!enabled &&
isServiceRunning())
I'd feel safer is isServiceRunning() was called just once.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:368: private BroadcastReceiver
receiver = new BroadcastReceiver() {
Brace on a new line please.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:370: public void onReceive(final
Context context, Intent intent)
Why make context final?

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

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

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

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

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/SummarizedPreferences.java:22: // initialize list
summaries
Why not call initSummaries() initListSummaries() that'd make the comment
redundant.

[Andrey Novikov, 2012-10-12 13:19:14]

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

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AboutDialog.java:20: private static Context mContext
= null;
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> What's with the "m"? The rest of the code base doesn't use a specific prefix
for
> denoting member variables.

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AboutDialog.java:83: {
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> I don't think we can be sure that reading the files works unconditional. I'm
in
> favour of a RuntimeException and/or a log message here.

Ok, whould log.

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

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:36: private final static
String TAG = "AdvancedPreferences";
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> How about "static final" instead of "final static"? That'd be consistent with
> the SUN/Oracle conventions and the line below.

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:61:
prefUpdate.setOnPreferenceClickListener(new OnPreferenceClickListener() {
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> How about putting this opening brace on its own line?

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:71:
prefConfiguration.setOnPreferenceClickListener(new OnPreferenceClickListener() {
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Same as above, brace should be on its own line.

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:116: {
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> You know I don't like long methods, this is one :) I don't insist on this
being
> split up, but it would be easier for me to understand that way.

We worked out a compromise in adding more comments.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:118: switch (id)
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Why use a switch case if there's only one case handled? I'd prefer an if.

This is a well known Android code pattern, there can be more dialogs in future.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:123: try
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> The same code for getting the package info is in AboutDialog. Maybe this
> warrants a utility method that ignores the exception? In this case that would
> simplify the following lines to:
> 
> PackageInfo pi = Utils.getPackageInfo(this);
> versionCode = pi.versionCode;
> 
> But if it becomes utility code, we can't really ignore the exception safely
> anymore, we should throw a RuntimeException or log an error.

NameNotFoundException is thrown when a given package, application, or component
name can not be found. It definitely finds caller package info.

AboutDialog also reads versionName which is not needed here, that's why I do not
put it into utility function.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:144: if
(Build.VERSION.SDK_INT >= 12) // Honeycomb 3.1
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> I think this should be a constant, since it's also used above. Or better yet,
> set a boolean field in the constructor.

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:167:
messageText.setOnClickListener(new View.OnClickListener() {
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Brace on a new line please.

Done.

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

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:41: private final static String TAG
= "Preferences";
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> As in AdvancedPreferences, "static final"?

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:77: copyAssets();
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Why can this getPackageInfo fail while those in
AboutDialog/AdvancedPreferences
> can't?

It can not. But I prefer to be reinsured in this case:
AboutDialog/AdvancedPreferences just show a number to a user, she wouldn't die
if she will see -1 instead of 123, but here application fill break if it will
not get up to date assets.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:91: public void onResume()
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Another long method, same comment as for AdvancedPreferences.onCreateDialog().

Same comment :) See code comments - they are for you.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:132:
subscriptionList.setOnRefreshClickListener(new View.OnClickListener() {
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Brace on its own line please.

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:155: (new Thread() {
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Brace on its own line please.

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:172: enabled = false;
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> enabled isn't read past this point, Setting it to false here is not necessary

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:270: Log.e(TAG, e.getMessage());
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Why not pass e as the third parameter, like below?

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:274: InputStream in = null;
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> in and out can be declared in the try block, they are not referenced outside
it.

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:288: in = null;
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> You don't need to set in/out to null, they aren't used anymore and will go out
> of scope very soon.

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:307: final Intent intent = new
Intent(Intent.ACTION_VIEW, uri);
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Why make intent final?

No reason, just copied url opening code from sample.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:322: showAbout = false;
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Naming nit pick: This sounds imperative, how about "aboutShown"?

For me name is correct - it flags if have to show about dialog on the next run.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:338: else if (!enabled &&
isServiceRunning())
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> I'd feel safer is isServiceRunning() was called just once.

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:368: private BroadcastReceiver
receiver = new BroadcastReceiver() {
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Brace on a new line please.

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/Preferences.java:370: public void onReceive(final
Context context, Intent intent)
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Why make context final?

It was previously used in a new thread, fixed.

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

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/RefreshableListPreference.java:29: final ViewGroup
widgetFrameView = ((ViewGroup) view.findViewById(android.R.id.widget_frame));
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Why is widgetFrameView final?

Done.

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/RefreshableListPreference.java:34: final float
mDensity = getContext().getResources().getDisplayMetrics().density;
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Why prefix this variable with "m"? It's local.

Copy/paste problems.

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

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/SummarizedPreferences.java:22: // initialize list
summaries
On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> Why not call initSummaries() initListSummaries() that'd make the comment
> redundant.

The comment was wrong, I've coped it from my old code. :)

[Felix Dahlke, 2012-10-12 13:29:46]

LGTM!

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

http://codereview.adblockplus.org/8493083/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/AdvancedPreferences.java:123: try
On 2012/10/12 13:19:14, Andrey Novikov wrote:
> On 2012/10/09 14:27:29, Felix H. Dahlke wrote:
> > The same code for getting the package info is in AboutDialog. Maybe this
> > warrants a utility method that ignores the exception? In this case that
would
> > simplify the following lines to:
> > 
> > PackageInfo pi = Utils.getPackageInfo(this);
> > versionCode = pi.versionCode;
> > 
> > But if it becomes utility code, we can't really ignore the exception safely
> > anymore, we should throw a RuntimeException or log an error.
> 
> NameNotFoundException is thrown when a given package, application, or
component
> name can not be found. It definitely finds caller package info.
> 
> AboutDialog also reads versionName which is not needed here, that's why I do
not
> put it into utility function.

That's why I suggested to return a PackageInfo object instead of just the
versionCode.