Peter_vdL

My Favorite Bug #47

by Peter_vdL Motorola 08-10-2011 06:13 AM - edited 08-14-2011 07:37 AM

They say you shouldn't play favorites, but I just can't help having a collection of favorite bugs.  I was reminded of this on vacation in Hawaii last week (see avatar, above left).  I was idly web surfing, when I looked around and found this large gecko observing me closely from the wall.

gecko.jpg

    A gecko appreciates bugs on a whole new level.

 

Now there's a guy that really appreciates bugs!  This gecko has his favorite bugs, and all of them have 6 legs.  I have my favorite bugs, and all of mine make me "unscrew the unscrutable", so to speak.  In this post, I describe a bug that will knock your socks off (all 6 legs).  The bug was brought to my attention by MOTODEV member and developer Noisygecko, who works on Android software at Georgia Tech Research Institute in Atlanta, GA.  The bug manifested as a problem with a preference setting for screen orientation.  

 

App Preferences

Here's a brief summary of Preferences in Android.  Preferences are user customizations, like font size, or audio volume, or landscape/portrait screen preference.  Preferences (user choices) are saved across different runs of an app.   To provide consistency across all apps, there are some framework classes in the android.preference package to store/edit/retrieve preferences. 

 

You can retrieve a preference with a call providing the string name of the preference, and the default value to use (if this preference has not been set yet).   A preference can be one of several atomic types, like float, boolean, int, and also String. 

 

   Here's how you retrieve a preference:

You get a SharedPreferences object with a static call.  Then invoke one of several get*() methods on the shared preference, supplying the string name of the preference you want.  You also provide the default value to return, if this preference hasn't been set yet, like this:

import android.preference.PreferenceManager.*
// ...
SharedPreferences pref =
SharedPreferences.getDefaultSharedPreferences( getContext() );
int orientPref = pref.getInt("orientation", DEFAULT_VALUE_CONST );

 

   Here's how you set some preference value:  

You get a SharedPreferences object, and use that to get a preference editor object, and use that to save a string key/value pair.  After putting one or more preference values, you commit() the change, and it's written to the app's preference file.

int orientPref = SOME_VALUE;
SharedPreferences pref = 
SharedPreferences.getDefaultSharedPreferences ( getContext() );
SharedPreferences.Editor prefEditor = pref.edit(); prefEditor.putInt("orientation", orientPref); prefEditor.commit();

You can add a SharedPreferenceChangeListener so that you get an event when someone else changes a preference that affects your app.   When your app has many settings, you can easily create a screen that supports editing them all.  Do this by extending PreferenceActivity.   Finally, there is support in the android.preference.PreferenceManager class for loading preferences from an XML file.  There's a great tutorial on Android preferences here.   

 

What was wanted to happen

This particular app wanted the screen orientation to be managed as a preference.  There was an options menu item that let the user toggle between landscape/portrait screen orientation preference.  Since the app only had one preference, there was no need to break this out into a separate PreferenceActivity.  The event handler for the options menu item did two things: 

  1. It set the orientation preference (as shown above), so the value would be remembered for the next run.
  2. It called a method in the Activity, to change to the requested screen orientation.  That method is
void setRequestedOrientation(int requestedOrientation)

The argument to setRequestedOrientation is an int.   You'll use a constant value, defined elsewhere in the Android framework, to express either "LANDSCAPE" or "PORTRAIT".      Hmm, where is that constant defined?  Screen orientation is part of the Configuration object used by the Alternative Resources framework, so let's take a look at class android.content.res.Configuration.   Yes, that seems to have the constants we need:

 

         code sample from android.content.res.Configuration

package android.content.res;
public class Configuration {
    public static final int ORIENTATION_UNDEFINED = 0;
    public static final int ORIENTATION_PORTRAIT = 1;
    public static final int ORIENTATION_LANDSCAPE = 2;

 

so, in the event handler for the menu item that lets the user select landscape screen orientation, we should do

  setRequestedOrientation(Configuration.ORIENTATION_LANDSCAPE);



No compile time errors, so proceed to testing.

 

What actually happened

Everything worked fine when the preference was set to portrait mode.  A problem happened when the user set the orientation preference to landscape.  Then the layout would flip back and forth between landscape and portrait orientations.



The bug analysis

This was one of those bugs that you either see through rightaway, or you burn excessive amounts of energy going over everything repeatedly, without seeing anything wrong.    Most people's first guess is something easy, like forgetting the manifest clause to prevent the activity from being restarted (for the stated config changes). Nope - that doesn't really match the symptom.  And the clause was in the manifest, so the Activity itself will handle orientation changes, just as we want.
<activity  ...  android:configChanges="orientation">
The programmer who encountered the problem, Noisygecko, put in log statements to publish everything of interest about the feature that was going wrong (setting the screen orientation to landscape).  Luckily, that quickly led him to the error! It turns out that android.app.Activity.setRequestedOrientation() takes an int argument, but the int constants are from class android.content.pm.ActivityInfo, not from android.content.res.Configuration!  Here is the ActivityInfo class.

         code sample from android.content.pm.ActivityInfo

package android.content.pm;
public class ActivityInfo {
    public static final int SCREEN_ORIENTATION_UNSPECIFIED = -1;
    public static final int SCREEN_ORIENTATION_LANDSCAPE = 0;
    public static final int SCREEN_ORIENTATION_PORTRAIT = 1;
    public static final int SCREEN_ORIENTATION_USER = 2;
With these very similar int constants in two different Android classes, the portrait value happens to have the same value in both classes.  But the landscape value does not!  And that's why the portrait case was working, and the landscape case was not.

The bug fix

The bug fix was a one liner.  Change the event handler, for the menu item that lets the user select "landscape screen orientation", to

 setRequestedOrientation(ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE);

(Yes, and change the portrait orientation constant too, to come from the right class).  Everything now worked perfectly.   It's amazing that I haven't written this bug, or one just like it, into my own code before!


How the API should have been designed
In fairness, the DroidDoc for android.app.Activity.setRequestedOrientation() does point you to the class where the right constants are defined.
But I should also point out that this part of the Android API is not well designed -- these constants should be defined in an enum, so the compiler can report mismatches like this.  Enums have been part of Java since JDK 1.5 in 2004 (about the time Android was a gleam in Andy Rubin's eye).  An enum to represent screen orientations would look like this:
public class ActivityInfo {

   public enum ScreenOrientation {
       BEHIND,
       FULL_SENSOR,
       LANDSCAPE,
       NOSENSOR,
       PORTRAIT,
       REVERSE_LANDSCAPE,
       REVERSE_PORTRAIT,
       SENSOR,
       SENSOR_LANDSCAPE,
       SENSOR_PORTRAIT,
       UNSPECIFIED,
       USER
       };

   public static ScreenOrientation current = ScreenOrientation.UNSPECIFIED;

}

The android.content.res.Configuration class could either re-use this enum, or define its own, if refactoring shows little overlap between the types.  But you could not use a literal from one enum in a place where a value from another enum is expected.  Under this approach, Activity would have a method with this signature:

void setRequestedOrientation(ScreenOrientation requestedOrientation)

This simple improvement would have no significant performance impact, and would make an entire class of errors reportable by the compiler.   Why wasn't it done this way?  I can't say for certain, but the way the code stands now, it bears the hallmarks of being written by someone whose first language is C or C++, not Java.  The original author probably just missed the right way to do something in Java.

 

Calling all developers!

My thanks go to Noisygecko for sharing this case study with me.   Do you have a favorite bug in your Android code that stands out in some way?  Something that was hard to solve or led to a surprising resolution?  Do you have a particularly good debugging approach?

 

Please follow up with a comment below, or a private message to me, and if I can blog about it, I will do, showcasing your work, and illuminating other Android developers!  Cheers,

 

Peter van der Linden                  See all my MOTODEV blog posts at: http://bit.ly/n1G8RD

Android Technology Evangelist

 

Comments
by Andy Tai(anon) on 08-12-2011 06:47 PM

Once one speaks C and C++, it is hard to speak Java without accent...

by Prakash(anon) on 08-13-2011 12:01 PM

I am not an expert in Java, but I kinda agree that that the constants are int and not enums with respect to problems I have encontered.

 

Enums in Java works well as long as all the Classes understands it correctly, Once you have to start serialize the enum then it becomes a problem. i.e. Socket, Filestream or SharedPreferences.

 

The Shared Preferences does not take enum value to write, so you have to convert it to int using ordinal, and when it needs to be retrived convert back to enum value. It takes some unwanted code to convert emum to int and back.

 

And there too, the dev can easily make a programming error to convert it to wrong enum.

 

So I would say this is a programming error and not a Android platform bug.

by JasonF(anon) on 08-13-2011 12:03 PM

Thanks for sharing that!

by Jon Shemitz(anon) on 08-13-2011 12:20 PM

A lot of people have run into Android's strange reliance on ints instead of enums, and wondered why. I don't think youcan really blame it on a greater familiarity with C or C++ than on Java - after all, even poor old C has enums. Others have blamed it on architects who cut their Java teeth before Java had enums, but I don't think there's much to that either. Surely, in a company that averages as young and brash as Google, someone would have said "You know, Java does have enums."

 

The true answer is that, unfortunately, it's not true that using enums "would have no significant performance impact" because Java enums are very very strange. Most languages treat enums as a special sort of automatically assigned integer. They offer varying degrees of compile-time type safety, but at runtime an enum is just an integer, and costs no more than any other integer. But, in Java, enums values are actually references to class instances.

 

This gives you type safety, and various minor benefits over enums in more normal languages, but it does mean that declaring lots of system constants as enums has significant costs in a mobile device with relatively little memory and comparatively slow CPU. Each enum value takes memory; each enum value has to be initialized.

 

I say this is the true answer not just because it makes sense to me, but also because Dianne Hackborn says so - http://markmail.org/message/7zgsfrefvyota2sf.

by Bob Murphy(anon) on 08-13-2011 12:39 PM

"[T]he way the code stands now, it bears all the hallmarks of being written by someone whose first language is C or C++, not Java."

 

Modern C and C++ also let you apply strong type checking to enums - I could have written a C++ class a lot like the example Java class Peter wrote with the embedded enum back in 1991.

 

The most common C/C++ equivalent to that bad API definition would be more like a block of preprocessor symbol definitions (e.g. #define ORIENTATION_UNDEFINED 0 etc.), and I'd consider that a code smell in anything written since about 1995. You could also write a bunch of const statics in a C++ class definition, almost exactly equivalent to the API definition, and I'd consider that an even worse smell, because at least the #define blocks are used in older code (e.g. the X server, etc.).

 

I think using enums the way Peter suggests to is a "best practice". Then the compiler would stop cold and scream bloody murder if you tried to assign between the different enums in the two classes. That's a much more efficient way to find a bug than to have to hunt it down at runtime.

 

by FlashSheridan 08-13-2011 01:01 PM - edited 08-13-2011 03:51 PM

Ahem.  This would be bad C code as well, and a good static analyzer would pick up Peter’s saner version.  (I’m not sure whether -Wextra would complain as well.)

by Dave Roseman(anon) on 08-13-2011 01:28 PM

Thank you, Peter.  You're right on the mark, as usual.

by Peter_vdL Motorola on 08-13-2011 01:56 PM

Thanks for the significant additional info, Jon.

 

    Peter

by Anton Spaans(anon) on 08-13-2011 02:25 PM

This particular issue is not a bug. And the documentation is pointing you to the right way.

However, this API could have been better designed to avoid confusion as described in your article.

 

About enums:

They do take up a bit more memory and require somewhat more processing power. However, this is minimal in current Android mobile devices. Enums should have been used in this case.

by mwk888(anon) on 08-13-2011 03:49 PM

Nice analysis, thanks Peter!

by Robert Allen(anon) on 08-14-2011 12:59 PM
Very interesting. I question, why do two such sets of parallel values even exist, instead of a single system or at least app wide singleton? To be fair though, the moment you mentioned 'finding this' in a header file I was on the alert for trouble. If it ain't from a man page, it's risky How I yearn for the elegant simplicity of original Version 7 UNIX :smileywink:
by Kai Wei(anon) on 08-14-2011 06:46 PM - last edited on 08-14-2011 09:46 PM by Peter_vdL Motorola

Thanks, Peter, for sharing. I believe this is not the only one of this kind of problem in Android. I think this difficulty has a deep root. At one time, using variables was recommended as a better way than using macros. But it seems it doesn't save us from bugs or confusion, if we use similar names as aliases of values, and thus make (what should be) differently typed things interchangable.



by wujf on 08-15-2011 02:59 AM
Using int over enum would not have much performance gain in this case, and the orientation values are unlikely to get involved in a cpu-intensive computation task. It's kind of a over-optimization.
by Peter Farkas(anon) on 08-15-2011 09:33 AM

Nice analysis, nice write-up.  Thank you for posting it.

Post a Comment
Be sure to enter a unique name. You can't reuse a name that's already in use.
Be sure to enter a unique email address. You can't reuse an email address that's already in use.
reCAPTCHA challenge image
Type the characters you see in the picture above.Type the words you hear.
About MOTODEV Blog
The MOTODEV blog keeps you updated on mobile app development news from MOTODEV and the Android developer community.

Subscribe to our RSS feed Subscribe via RSS

Follow Us:
Fan MOTODEV on Facebook Join the MOTODEV LinkedIn Group MOTODEV on YouTube

motodev profile

motodev @david_latham what specifically are you trying to download? 7 days ago · reply · retweet · favorite

motodev profile

motodev RT @StackMob: Join StackMob and Salesforce at the MOTODEV Google + Hangout today at noon. Register here l2cloud-stackmob.eventbrite.com 7 days ago · reply · retweet · favorite

motodev profile

motodev G+ Hangout: Migrating ur legacy systems to the cloud & mobile is at @ 12pm PDT today. Join us and bring your questions. #l2cloud 7 days ago · reply · retweet · favorite

Our Blog & Comment Policy
Opinions expressed here and in any corresponding comments are the personal opinions of the original authors, not of Motorola. The content is provided for informational purposes only and is not meant to be an endorsement or representation by Motorola or any other party.

Remember, when you comment, please stay on topic and avoid spam, profanity, and anything else that violates our user guidelines. All comments require approval by Motorola and can be rejected for any reason.

For customer support issues with your Motorola phone go to the Motorola customer support website.