Starting with honeycomb, prefs use a two paned style. We should too?

That's just a WIP patch. Screenshot of the General tab on a tablet. A few things to finish up (localization, and removing the duplicate labels on Honeycomb and up devices), and probably more questions about what we want to do. Is this the grouping we would want? Do we want this at all. I think... after looking at this that maybe we should just have a general tab with 3 sub sections 1a.) About Firefox 1b.) Sync 2.) Content 2a.) Character Encoding 2b.) Plugins 2c.) Font Inflation 3.) Import & Export 3a.) Android and a separate screen for the privacy stuff which is getting unweildy right now. But maybe at that point this whole exercise gets useless... A few more screenshots from other devices coming...

This fixes the multiple labels bit and keeps us using our current style prefs (single screen) on ICS phones. I think I'm gonna mix up the General, Content and Import/Export prefs on tablets and then put this up for review if UX hasn't commented yet.

Comment on attachment 642619 [details] [diff] [review] WIP Patch v2 >diff --git a/mobile/android/base/GeckoPreferences.java b/mobile/android/base/GeckoPreferences.java > protected void onCreate(Bundle savedInstanceState) { >+ if (Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB || !onIsMultiPane()) { >+ addPreferencesFromResource(R.xml.preferences); >+ } {} not needed > GeckoAppShell.registerGeckoEventListener("Preferences:Data", this); > GeckoAppShell.registerGeckoEventListener("Sanitize:Finished", this); >+ public void initGroups(PreferenceGroup preferences, ArrayList<String> prefs) { > if (pref instanceof PreferenceGroup) >- initGroups((PreferenceGroup)pref); >+ initGroups((PreferenceGroup)pref, prefs); > else { Not your code, but the if block needs {} because the else block has them > private void refresh(JSONArray jsonPrefs) { >+ Log.i(LOGTAG, "Refresh"); > try { >- if (mPreferenceScreen == null) >+ if (mPreferenceScreen == null) { >+ Log.i(LOGTAG, "No screen"); > return; Let's drop the extra logging for checkin > } >+ > } Blank line not needed >diff --git a/mobile/android/base/GenericPrefs.java b/mobile/android/base/GenericPrefs.java I am not the biggest fan of prefixing "Gecko" on all the code, and I guess we don't need to with Java namesapcing, but until we make GeckoPreferences -> Preferences maybe we should call this GeckoPreferenceFragment >\ No newline at end of file Needs newline f+, I like the direction. Let's see what UX thinks.

I'm moving forward with this, but will ping ibarlow in the morning for UX feedback. For now, I like it like this. Things (like tablets) that will show two panes have a General and Privacy section. Things like phones that have one pane should look the same as they do now. I'll attach screenshots from my three devices again.

Whoops. The old patch had some issues. I also fixed some problems with the "home" button in the upper left not working on ICS and not having an icon on honeycomb.

Build for UX http://people.mozilla.com/~wjohnston/tabletPrefs.apk

Hi Wes, this is looking good! I am seeing a bit of weirdness on my Xoom running ICS, maybe you can look into it a bit: Usually the layout is fine, but if I rotate the screen and change tabs, the list alignment messes up. The top rule ends up with different indentation than the others: http://cl.ly/image/3P2P0C3t3915 And this indentation ends up with text getting cut off in portrait mode here: http://cl.ly/image/0w0e0T1y2d0T

Comment on attachment 643212 [details] [diff] [review] Patch v2 Holding off on review until you can look at Ian's issues.

No changes. I just unbitrot these each week and wanted to make sure the updated ones where here. I dug into the Android source briefly and haven't seen any way to control these paddings. The string problems are just the result of us having long strings. We'll have to shorten them. Or write a custom preference type (two lines or a smaller font)?

I unbitrotted this, and can't reproduce the bug with the padding. Even if I could, I think its basically out of our hands. mfinkle suggested we try moving this forward again, so putting up for review.

Comment on attachment 711071 [details] [diff] [review] Update patch Review of attachment 711071 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoPreferenceFragment.java @@ +13,5 @@ > +import android.util.Log; > + > +public class GeckoPreferenceFragment extends PreferenceFragment { > + > + private PreferenceScreen mPreferenceScreen; Nit: remove empty line after class declaration ::: mobile/android/base/GeckoPreferences.java @@ +78,5 @@ > + > + @Override > + public void onBuildHeaders(List<Header> target) { > + if (onIsMultiPane()) > + loadHeadersFromResource(R.xml.preferences_headers, target); Nit: remove trailing whitespace ::: mobile/android/base/locales/en-US/android_strings.dtd @@ +120,1 @@ > <!ENTITY pref_private_data_downloadFiles "Downloaded files"> Nit: move these two after "Downloaded files" ::: mobile/android/base/resources/xml/preferences_privacy_tablet.xml.in @@ +8,5 @@ > +<PreferenceScreen xmlns:android="http://schemas.android.com/apk/res/android" > + xmlns:gecko="http://schemas.android.com/apk/res/@ANDROID_PACKAGE_NAME@" > + android:enabled="false"> > + > +#include preferences_privacy.xml General question about these #includes: Will these make it more difficult to debug any XML errors? These preprocessor includes will generate one big XML file before being parsed, and having to check the android build directory to see the output could be annoying. I guess it's fine if this is our only option, but I'm curious about the approach you used earlier that mfinkle gave f+. In that patch, you were using <include /> tags instead of #include. Any reason we can't switch these to use that instead? IMO, <include /> also looks cleaner because you can keep the correct indentation where it's being used.

(In reply to Brian Nicholson (:bnicholson) from comment #19) > Will these make it more difficult to debug any XML errors? These > preprocessor includes will generate one big XML file before being parsed, > and having to check the android build directory to see the output could be > annoying. I guess it's fine if this is our only option, but I'm curious > about the approach you used earlier that mfinkle gave f+. In that patch, you > were using <include /> tags instead of #include. Any reason we can't switch > these to use that instead? IMO, <include /> also looks cleaner because you > can keep the correct indentation where it's being used. Near as I can tell, my original patch never actually worked. Unbitrotting it, it crashes on run because <include> is really only allowed in layout files. These preferences files are not layouts. (java.lang.ClassNotFoundException: android.preference.include). I think this was the only way to make this work.

bnicholson ping?

If you don't mind, could you try throwing together something similar to https://github.com/commonsguy/cw-android/tree/master/Prefs/FragmentsBC/src/com/commonsware/android/preffragsbc so that we can compare the two implementations? The #include web with mini XML pieces in this patch feels kind of awkward, and if I understand PreferenceFragments correctly, they were designed to handle exactly this problem. Can we see if there's a way to separate just the two sections we want for each screen? If you're busy, I could try doing this myself to see what it looks like.

This goes on top of the other patch, and doesn't clean up the files it doesn't use anymore, but gives an idea what this would look like. The problem with this is a UX polish one (I'll post a screenshot). On tablets, we wind up showing the categories three times on the page (two of which are expected). Once on the left pane. Once at the top of the right pane, and once (again) at the top of the right pane where the actual PrefCategory is. I can dig and try to get access to the category, but even if I do, changing/hiding the title isn't something you can do with it. i.e. we need to just not have the PrefCategory present on tablets, but we do want it on phones. At build time duplicating the content into two different files is the best way to do that I can think of? Alternatively, maybe we could play with renaming things so that the same string isn't duplicated.

Maybe we could throw "Import from Android" into General (we don't have the ability to export anyway so "Import & Export" doesn't make much sense)? Then we could have three tabbed sections: General, Content, and Privacy?

I'm fine with that, but it still doesn't fix the doubled up labels problem.

I played with this a bit yesterday and came up with something that seems to work. What do you think of this patch (it applies on top of the first 2)? Basically, it creates a new PreferenceScreen, iterates through the preferences in the original PreferenceScreen, and adds the old prefs to the new screen (excluding categories). Combined with the minor pref reorganization discussed above, this should allow us to use the simplified XML approach. To measure performance, I benchmarked these lines: + mPreferenceScreen = stripCategories(getPreferenceScreen()); + setPreferenceScreen(mPreferenceScreen); and saw 0ms. This is on an old ASUS Transformer, so I don't think performance should be a concern.

Comment on attachment 718516 [details] [diff] [review] Strip PreferenceCategories from PreferenceScreen Heh. Interesting. This strips all categories it looks like, when we just want to strip the first one. It should also only run on tablets. But I get the idea. I don't really think the added code complexity (what you see on screen no longer reflects whats in the XML) is worth the tiny savings in debugging xml errors. But I'll concede if it helps move this forward.

(In reply to Wesley Johnston (:wesj) from comment #28) > Comment on attachment 718516 [details] [diff] [review] > Strip PreferenceCategories from PreferenceScreen > > Heh. Interesting. This strips all categories it looks like, when we just > want to strip the first one. What do you mean we just want to strip the first one? I though we'd have each section of preferences goes in its own PreferenceScreen (General, Content, and Privacy or something), and then each one would get its own header. That way, there would be exactly one category per header, and the division of prefs would be the same on tablets as it is on phones. > I don't really think the added code complexity (what you see on screen no > longer reflects whats in the XML) is worth the tiny savings in debugging xml > errors. But I'll concede if it helps move this forward. I guess I'm not concerned with just the debugging errors - IMO, having a PreferenceScreen for each section is cleaner in general because of the reduced preprocessing directives. But I don't want to force you to go with this approach just because I've worn you down :) If you want, you can ask mfinkle or someone to compare the two, and I'll just r+ the first one if I'm outnumbered.

I'd be fine with having one section per set if we move Import into General (which I think we all agree is good).

Comment on attachment 711071 [details] [diff] [review] Update patch Cancelling review since Wes is changing his patch to use the runtime approach.

Pulled this together with your stripping category stuff (neat!). My tablet is dead. This works fine on my phone in normal mode and when I remove the onIsMultiPane checks, and essentially get the tablet UI.

Comment on attachment 736530 [details] [diff] [review] Patch Review of attachment 736530 [details] [diff] [review]: ----------------------------------------------------------------- Lots of questions; r- for some cleanup issues and to get questions answered. ::: mobile/android/base/GeckoPreferenceFragment.java @@ +20,5 @@ > +*/ > +public class GeckoPreferenceFragment extends PreferenceFragment { > + > + private PreferenceScreen mPreferenceScreen; > + private ArrayList<String> mPreferencesList; No need for either of these to be class-level variables @@ +21,5 @@ > +public class GeckoPreferenceFragment extends PreferenceFragment { > + > + private PreferenceScreen mPreferenceScreen; > + private ArrayList<String> mPreferencesList; > + private String LOGTAG = "GeckoPreferenceFragment"; Make this static @@ +25,5 @@ > + private String LOGTAG = "GeckoPreferenceFragment"; > + > + public GeckoPreferenceFragment() { > + super(); > + } Drop this constructor @@ +38,5 @@ > + addPreferencesFromResource(res); > + > + mPreferencesList = new ArrayList<String>(); > + /* This is only hit when we're using the headers (i.e. on large screen devices). > + Strip the first category there is isn't shown twice */ s/is isn't/so it isn't/ ? @@ +43,5 @@ > + mPreferenceScreen = stripCategories(getPreferenceScreen()); > + setPreferenceScreen(mPreferenceScreen); > + > + GeckoPreferences activity = (GeckoPreferences)getActivity(); > + activity.setScreen(mPreferenceScreen); Why is it necessary to set the activity's PreferenceScreen to the fragment's PreferenceScreen? I thought each PrefrenceFragment had its own PreferenceScreen, and they were nested inside of (and not equivalent to) their parent's PreferenceScreen? ::: mobile/android/base/GeckoPreferences.java @@ +90,4 @@ > } > > @Override > public void onWindowFocusChanged(boolean hasFocus) { I don't understand why we do this initialization in onWindowFocusChanged(). We don't even have an initialized flag, so I think initialization will happen every time we gain/lose focus (e.g., open a dialog pref, then click back). Do you think we can just merge this into onCreate()? @@ +95,5 @@ > return; > > mPreferencesList = new ArrayList<String>(); > + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB || !onIsMultiPane()) { > + mPreferenceScreen = getPreferenceScreen(); mPreferenceScreen is already set in onCreate() -- why do we have to do it again here? @@ +96,5 @@ > > mPreferencesList = new ArrayList<String>(); > + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB || !onIsMultiPane()) { > + mPreferenceScreen = getPreferenceScreen(); > + if (mPreferenceScreen != null) { The API for getPreferenceScreen() doesn't say it can return null. Do we need this check? If so, what happens if it is null? Do we just show nothing? @@ +98,5 @@ > + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB || !onIsMultiPane()) { > + mPreferenceScreen = getPreferenceScreen(); > + if (mPreferenceScreen != null) { > + initGroups(mPreferenceScreen, mPreferencesList); > + initValues(mPreferencesList); You have these 2 lines both here and in GeckoPreferencesFragment. Rather than only doing this here for small screen phones, could we do initGroups() and initValues() unconditionally? Then ideally we wouldn't need these calls in GeckoPreferencesFragment, and we could keep initGroups() and initValues() private. If these are being called too early for fragments to work, could we at least have initGroups/initValues in onBuildHeaders() instead of GeckoPreferencesFragment (so they could still remain private)? @@ +151,5 @@ > + mPreferenceScreen = screen; > + } > + > + public void initGroups(PreferenceGroup preferences, ArrayList<String> prefs) { > + //final int count = preferences.getPreferenceCount(); Drop this @@ +161,5 @@ > pref.setOnPreferenceChangeListener(this); > + String key = pref.getKey(); > + > + if (!AppConstants.MOZ_UPDATER) { > + if (key != null && key.equals(PREFS_UPDATER_AUTODOWNLOAD)) { Nit: you can remove this null check by doing "PREFS_UPDATER_AUTODOWNLOAD.equals(key)" @@ +163,5 @@ > + > + if (!AppConstants.MOZ_UPDATER) { > + if (key != null && key.equals(PREFS_UPDATER_AUTODOWNLOAD)) { > + preferences.removePreference(pref); > + i--; Could you have a continue statement here? Doesn't seem like you'd want the pref to be added to the prefs array below. @@ +167,5 @@ > + i--; > + } > + } > + > + if (key != null && key.equals(PREFS_TELEMETRY_ENABLED)) { Nit: you can remove this null check by reversing the equality comparison @@ +170,5 @@ > + > + if (key != null && key.equals(PREFS_TELEMETRY_ENABLED)) { > + if (AppConstants.MOZ_TELEMETRY_REPORTING) { > + if (AppConstants.MOZ_TELEMETRY_ON_BY_DEFAULT) { > + pref.setKey(PREFS_TELEMETRY_ENABLED_PRERELEASE); To be safe, you should probably also set the "key" variable here to the same thing. @@ +174,5 @@ > + pref.setKey(PREFS_TELEMETRY_ENABLED_PRERELEASE); > + } > + } else { > + preferences.removePreference(pref); > + i--; Could you have a continue statement here? Doesn't seem like you'd want the pref to be added to the prefs array below. ::: mobile/android/base/resources/xml/preference_headers.xml @@ +3,5 @@ > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<preference-headers > + xmlns:android="http://schemas.android.com/apk/res/android"> Nit: don't split these lines ::: mobile/android/base/resources/xml/preferences_content.xml @@ +1,5 @@ > +<?xml version="1.0" encoding="utf-8"?> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + Nit: trailing whitespace (here and in several places below in this file) ::: mobile/android/base/resources/xml/preferences_general.xml @@ +9,5 @@ > + > + <PreferenceCategory android:title="@string/pref_category_general"> > + > + <org.mozilla.gecko.LinkPreference android:title="@string/pref_about_firefox" > + url="about:" /> Not your bug, but should this be gecko:url? The lack of a namespace here seems weird. @@ +10,5 @@ > + <PreferenceCategory android:title="@string/pref_category_general"> > + > + <org.mozilla.gecko.LinkPreference android:title="@string/pref_about_firefox" > + url="about:" /> > + Nit: trailing whitespace (here and in several places below in this file) ::: mobile/android/base/resources/xml/preferences_privacy.xml @@ +13,5 @@ > + android:title="@string/pref_cookies_menu" > + android:entries="@array/pref_cookies_entries" > + android:entryValues="@array/pref_cookies_values" > + android:persistent="false" /> > + Nit: trailing whitespace (here and in several places below in this file)

The onWindowFocusChanged stuff is (apparently) a startup improvement of 100ms (bug 726732). The mPreferenceScreen bit is kinda strange. I'm not sure why we ever put it in to begin with. Its not needed with our current setup (PreferenceActivity already holds a reference for us). But with fragments, I don't think getPreferenceScreen() is guaranteed to return anything, so with this new patch... things get stranger (like you said, the docs don't say anything, and the API is deprecated, I don't really want to depend on it). For now, we have code that depends on mPreferenceScreen being set, and so the fragments set it. I'll try to kill it in a second patch... maybe in a second bug because its likely to cause problems entirely unrelated to this. initGroups() and initValues() have to be called for each screen because each screen contains a different set of prefs. They're horribly named don't do what you'd think they do. I'll rename them and clean up a bit.

(In reply to Wesley Johnston (:wesj) from comment #34) > The onWindowFocusChanged stuff is (apparently) a startup improvement of > 100ms (bug 726732). 100ms seems fairly minor, especially since there's not really much to be gained from loading this screen sooner (you can't actually interact with it until this code has been called anyway). I wouldn't mind if you just dropped the onWindowFocusChanged() code and moved it into onCreate(). But if you want to keep it, we should add a boolean to prevent initialization from happening repeatedly; the window can gain and lose focus repeatedly while the same activity is alive. > The mPreferenceScreen bit is kinda strange. I'm not sure why we ever put it > in to begin with. Its not needed with our current setup (PreferenceActivity > already holds a reference for us). Yeah, I agree that we should get rid of it. It would simplify your patch a bit since you don't have to pass it around. > initGroups() and initValues() have to be called for each screen because each > screen contains a different set of prefs. They're horribly named don't do > what you'd think they do. I'll rename them and clean up a bit. Oh, I see. Do you think we could move this initGroups/initValues stuff into a separate class that's responsible for populating the prefs from Gecko? Then we could delegate this initialization in GeckoPreferences and GeckoFragmentPreference by calling the helper class? That way, we wouldn't have this weird coupling between the fragment and the PreferenceActivity, which we currently have to cast to GeckoPreferences and call these methods on. Or we could just handle this all in a separate prefs refactor bug since that code is such a mess right now.

Fixed up patch. I left the helper class thing out. We can do that in a separate bug maybe? but I don't really have a problem with this class cast.

Comment on attachment 738156 [details] [diff] [review] Patch Wrong patch

Grr. My patch queue is fighting me. There's an override OnIsMulti() in there that is removed locally.

Added back the default listener, plus a little bit of more cleanup I found in the process.

Comment on attachment 738281 [details] [diff] [review] Patch Review of attachment 738281 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoPreferenceFragment.java @@ +42,5 @@ > + PreferenceScreen newScreen = getPreferenceManager().createPreferenceScreen(preferenceScreen.getContext()); > + int order = 0; > + if (preferenceScreen.getPreferenceCount() > 0 && preferenceScreen.getPreference(0) instanceof PreferenceCategory) { > + PreferenceCategory cat = (PreferenceCategory) preferenceScreen.getPreference(0); > + for (int j = 0; j < cat.getPreferenceCount(); j++) { Nit: use i instead of j ::: mobile/android/base/GeckoPreferences.java @@ +94,5 @@ > > + mInitialized = true; > + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB || !onIsMultiPane()) { > + PreferenceScreen screen = getPreferenceScreen(); > + if (screen != null) { I still don't understand why this check is here. Did you read that getPreferenceScreen() can return null somewhere? It doesn't say anything about that in the Android API. Even if it is null, do we do anything to try to handle it or just show an empty prefs screen? If the latter, I'd be inclined to just let the NPE crash happen so we can track the bug and fix it; otherwise, people will just be left with a broken UI that doesn't get reported. ::: mobile/android/base/resources/xml/preferences_content.xml @@ +1,5 @@ > +<?xml version="1.0" encoding="utf-8"?> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + Nit: whitespace