Dialog to manage installed search providers

VERIFIED FIXED in Firefox 26

Status

()

defect
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: Margaret, Assigned: ckitching)

Tracking

(Blocks 1 bug)

Trunk
Firefox 26
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap +

Firefox Tracking Flags

(firefox26 verified, fennec25+)

Details

Attachments

(5 attachments, 30 obsolete attachments)

25.73 KB, patch
Details | Diff | Splinter Review
6.35 KB, patch
Details | Diff | Splinter Review
11.58 KB, patch
Details | Diff | Splinter Review
7.75 KB, patch
liuche
: review+
Details | Diff | Splinter Review
10.31 KB, patch
liuche
: review+
Details | Diff | Splinter Review
Reporter

Description

6 years ago
This will be follow-up work to bug 892094. We want to show a dialog when you tap on an installed search provider, giving you enable/disable/set as default options.
Assignee

Comment 1

6 years ago
I notice this dialog isn't in the pictures provided on Bug 891115 - I'd appreciate some input on how the layout in this dialog should be.
I guess we'll also want a "Delete" button for search providers?
Flags: needinfo?(ibarlow)
Hi Chris, sure.

The three main items in the menu should be


  +-------------------------------+
  |                               |
  | Set as default                |
  |                               |
  +-------------------------------+
  |                               |
  | Disable                       |
  |                               |
  +-------------------------------+
  |                               |
  | Uninstall                     |
  |                               |
  +-------------------------------+


* "Set as default" should be greyed this out if already the default
* If an engine has been disabled, that menu item should read "Enable"
Flags: needinfo?(ibarlow)
Assignee

Updated

6 years ago
No longer blocks: 892094
Depends on: 892094
Assignee

Comment 3

6 years ago
Surely it is the case that this patch depends on the existence of the search settings screen, not that it blocks it (You could, after all, create the settings screen without these buttons doing anything (I just did, in fact :P )).
Or am I misunderstanding the use of the flags? (It seems that my patch here will depend on the one for 892094, so isn't this relationship a dependency, not a blockage?)
Assignee

Comment 4

6 years ago
A patch in three parts:

1) Add the new UI to display the default engine and enabledness of engines in the list; a prompt to remove, toggle enabledness, and uninstall engines.
2) Have the new UI components induce the appropriate configuration changes in Gecko's search engine service (Altering it as needed to make this work)
3) Remove the now-obsolete facility for changing default search engine (via the addons menu - that horrid kludge)

Included: Part the first.
You can tap shiny buttons and they'll lie horridly about actually changing your settings while not actually changing your settings. Useful for seeing how the user experience is expected to be (Feedback request in case Ian wants to give this a spin and provide comments about the way this works - if you're not in the mood for applying a patch and compiling Fennec I can provide a series of screenshots/a video illustrating how this feels in a few hours.)

Part the second - coming today..
Attachment #775557 - Flags: review?(liuche)
Attachment #775557 - Flags: feedback?(ibarlow)
Assignee

Comment 5

6 years ago
Note: The patches here build on the work done for Bug 892094 - it is necessary to apply that patch first.
Hi Chris, I'm not equipped to make builds. I'm happy to try a build if you make one, or screenshots/video are great too, thanks.
Assignee

Comment 7

6 years ago
No problem - here's an apk with the patches installed for you:
https://www.dropbox.com/s/q5e9s0ajpd6ir94/fennec-25.0a1.en-US.android-arm.apk
Enjoy!
Assignee

Comment 8

6 years ago
Regenerated patch that will apply atop the revised parent patch.
Attachment #775557 - Attachment is obsolete: true
Attachment #775557 - Flags: review?(liuche)
Attachment #775557 - Flags: feedback?(ibarlow)
Attachment #776012 - Flags: review?(liuche)
Assignee

Updated

6 years ago
Attachment #776012 - Attachment description: Part 1/3: The shiny new UI, not actually talking to Gecko. V2 (Parent patch changed) → Part 1/3: The shiny new UI, not actually talking to Gecko. V2 (BORK).
Attachment #776012 - Attachment is obsolete: true
Attachment #776012 - Flags: review?(liuche)
Assignee

Comment 10

6 years ago
Uninstalling the default search engine now leads to a fallback being selected, instead of an undefined default engine.
Attachment #776028 - Attachment is obsolete: true
Attachment #776028 - Flags: feedback?(liuche)
Attachment #776047 - Flags: feedback?(liuche)
Assignee

Comment 11

6 years ago
Occasionally the enabledness properties would be reset by the fact that GeckoPreferences called setEnabled(true) on all elements when it finishes setup. No longer.
Attachment #776047 - Attachment is obsolete: true
Attachment #776047 - Flags: feedback?(liuche)
Assignee

Comment 12

6 years ago
Causes the buttons added in the previous patch to actually have their results saved by the Gecko engine. We have a nice way of changing search providers! Yay!
Attachment #776226 - Flags: review?(liuche)
Assignee

Comment 13

6 years ago
Removing the somewhat kludgeesque prior implementation. For now leaving the actual xml files used to define the bundled search engines as they are, as it is not entirely clear how best to refactor these (If we care to at all).
I note that the xml files in question actually include the favicons for the search engines inline - this is a somewhat wasteful method of encoding graphics - the apk could be made very slightly smaller if these were refactored out into seperate image files and loaded in the usual way.
Attachment #776228 - Flags: review?(liuche)
Assignee

Updated

6 years ago
Attachment #776224 - Flags: review?(liuche)
Assignee

Comment 14

6 years ago
(We also probably don't care too much about entirely correctly incrementally removing things from the current addons management screen, since my next task is, essentially, to get rid of it)

In fact, with that in mind, it might be worthwhile to just ignore the third patch here for now. At some later point we can just delete the offending js file entirely.
This is looking good, Chris. Couple comments:

Menu should read "Set as default". Right now it says "Set default".

The default search provider should just say "Default" under it, not "Default search engine". 

Turning search suggestions on and off seems to blow away the "default" label in the search provider list. This shouldn't be happening.
Reporter

Comment 16

6 years ago
(In reply to Chris Kitching [:ckitching] from comment #3)
> Surely it is the case that this patch depends on the existence of the search
> settings screen, not that it blocks it (You could, after all, create the
> settings screen without these buttons doing anything (I just did, in fact :P
> )).
> Or am I misunderstanding the use of the flags? (It seems that my patch here
> will depend on the one for 892094, so isn't this relationship a dependency,
> not a blockage?)

We usually use "block" for follow-up work, meaning that the search settings screen is the whole feature, and this is part of it. It doesn't really matter much, but it does make it show up in the meta bug's dependency tree, which is useful. However, we could also just set this to block the meta bug so that it shows up there :)
Reporter

Comment 17

6 years ago
(In reply to Chris Kitching [:ckitching] from comment #14)
> (We also probably don't care too much about entirely correctly incrementally
> removing things from the current addons management screen, since my next
> task is, essentially, to get rid of it)
> 
> In fact, with that in mind, it might be worthwhile to just ignore the third
> patch here for now. At some later point we can just delete the offending js
> file entirely.

We should check with product to see if we're comfortable just shipping the search bit in settings without the add-ons bit. If we want to ship these features together, it would be best to leave about:addons alone, so that we can fall back to using it if we don't think we'll be able to ship in 25.
Reporter

Comment 18

6 years ago
Comment on attachment 776224 [details] [diff] [review]
Part 1/3: The new UI, sans actually talking to Gecko. V5 - Eliminated race condition with inflaters.

Oops, looks like you accidentally uploaded the part 2 patch twice here.
Reporter

Comment 19

6 years ago
Comment on attachment 776226 [details] [diff] [review]
Part 2/3: Communication with Gecko to save the changes made via the UI.

Review of attachment 776226 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +6341,5 @@
> +        engine.hidden = false;
> +      break;
> +      case "SearchEngines:Disable":
> +        engine = this._extractEngineFromJSON(aData);
> +        engine.hidden = true;

Drive-by: I wonder if it would be worth combining these two messages to save on an observer. We could just make one "SearchEngines:Enable" message, and pass the enable/disable state as a boolean. Of course that would then require us to parse aData differently in this case.
Assignee

Comment 20

6 years ago
> ::: mobile/android/chrome/content/browser.js
> @@ +6341,5 @@
> > +        engine.hidden = false;
> > +      break;
> > +      case "SearchEngines:Disable":
> > +        engine = this._extractEngineFromJSON(aData);
> > +        engine.hidden = true;
> 
> Drive-by: I wonder if it would be worth combining these two messages to save
> on an observer. We could just make one "SearchEngines:Enable" message, and
> pass the enable/disable state as a boolean. Of course that would then
> require us to parse aData differently in this case.


I actually refactored from that to this for readability - are observers expensive, then, and something I should be trying to minimize?

(In reply to :Margaret Leibovic from comment #18)
> Comment on attachment 776224 [details] [diff] [review]
> Part 1/3: The new UI, sans actually talking to Gecko. V5 - Eliminated race
> condition with inflaters.
> 
> Oops, looks like you accidentally uploaded the part 2 patch twice here.


Drat.
Assignee

Updated

6 years ago
Attachment #776504 - Attachment description: 776224: Part 1/3: The new UI, sans actually talking to Gecko. V6 - Really eliminated race condition with inflaters. → Part 1/3: The new UI, sans actually talking to Gecko. V6 - Really eliminated race condition with inflaters.
Comment on attachment 776504 [details] [diff] [review]
Part 1/3: The new UI, sans actually talking to Gecko. V6 - Really eliminated race condition with inflaters.

Review of attachment 776504 [details] [diff] [review]:
-----------------------------------------------------------------

Is this the same patch as from bug 892094? If so, it doesn't need to be labeled 1/3, because it's technically not part of this bug. (If it is different, strip out the parts that have changed from the other bug.) In the future, just differentiate patch dependencies by specifying that this is a patch from bug <number> and it should be applied before patches for this bug.
Attachment #776504 - Flags: review?(liuche)
Assignee

Comment 23

6 years ago
Comment on attachment 776504 [details] [diff] [review]
Part 1/3: The new UI, sans actually talking to Gecko. V6 - Really eliminated race condition with inflaters.

Yes, of course. Wow. Impressively stupid. Managed to convince myself that this patch was in fact two patches.
Attachment #776504 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #776226 - Attachment description: Part 2/3: Communication with Gecko to save the changes made via the UI. → Part 1/2: Communication with Gecko to save the changes made via the UI.
Assignee

Updated

6 years ago
Attachment #776228 - Attachment description: Part 3/3: Eliminate the original method of changing search engine settings (Via the addons menu) → Part 2/2: Eliminate the original method of changing search engine settings (Via the addons menu)
Assignee

Comment 24

6 years ago
Ah! No! I'm not quite as much of an idiot as previously thought. There ARE three patches, but I uploaded the wrong one for number 1 previously.
So, in summary, there are:
1) Make the buttons work
2) Make the buttons actually talk to Gecko
3) Remove the old stuff.

But, in my haste, I accidentially uploaded the patch from 892116 in place of 1.

Right. Okay. Fine. Yay. That's how it's supposed to be.
Attachment #776706 - Flags: review?(liuche)
Assignee

Updated

6 years ago
Attachment #776706 - Attachment description: Part 1/3: Communication with Gecko to save the changes made via the UI. → Part 1/3: New UI. V7 (Race condition with GekcoPreferences)
Assignee

Updated

6 years ago
Attachment #776226 - Attachment description: Part 1/2: Communication with Gecko to save the changes made via the UI. → Part 2/3: Communication with Gecko to save the changes made via the UI.
Assignee

Updated

6 years ago
Attachment #776228 - Attachment description: Part 2/2: Eliminate the original method of changing search engine settings (Via the addons menu) → Part 3/3: Eliminate the original method of changing search engine settings (Via the addons menu)
A quick drive-by after playing with the applied patches:

- Add the string changes from Ian's comment 15
- Bug: when I disable all the search engines and then re-eanble the previous default, it remains grayed out
- Disabling all the search engines causes 404 when typing words into the urlbar. That probably indicates that you shouldn't be able to disable all the search engines.
Ian: I think the enable-disable-uninstall actions for search engines are really confusing, especially since there isn't explicit context for what enabling/disabling a search engine means, and where this would be. What do you think about something like "Show/Hide in search options" (choices?) and "Remove"?
Flags: needinfo?(ibarlow)
Assignee

Comment 27

6 years ago
Since the last engine is always the default at that point, and since we want to be unable to either disable or uninstall the final engine, the problem described is simply solved by declining to show the prompt when only one engine is left. (As all the buttons on it would be disabled anyway)
Is this behaviour okay, or does anyone have a better idea?
Attachment #776706 - Attachment is obsolete: true
Attachment #776706 - Flags: review?(liuche)
Attachment #777514 - Flags: review?(liuche)
(In reply to Chenxia Liu [:liuche] from comment #26)
> Ian: I think the enable-disable-uninstall actions for search engines are
> really confusing, especially since there isn't explicit context for what
> enabling/disabling a search engine means, and where this would be. What do
> you think about something like "Show/Hide in search options" (choices?) and
> "Remove"?

That's a good point. I suppose that language is carry-over from when Search lived inside of Add-ons, which makes even less sense now that we are separating them. Your suggestion for strings is certainly clearer, let's do it.
Flags: needinfo?(ibarlow)
Assignee

Comment 29

6 years ago
Part 2/3: Communication with Gecko to save the changes made via the UI. V2: Fighting bitrot.
Attachment #776226 - Attachment is obsolete: true
Attachment #776226 - Flags: review?(liuche)
Attachment #777973 - Flags: review?(liuche)
Assignee

Updated

6 years ago
Attachment #777973 - Attachment description: notifyGeckoOfSearchEngineChanges.patch → Part 2/3: Communication with Gecko to save the changes made via the UI.
Assignee

Updated

6 years ago
Attachment #777973 - Attachment description: Part 2/3: Communication with Gecko to save the changes made via the UI. → Part 2/3: Communication with Gecko to save the changes made via the UI. V2 - Fixing bitrot.
Assignee

Comment 31

6 years ago
Added the suggested changes to the labels for "Enabled"/"Disabled".

To my mind, "Show in search options" is a little confusing, too, because it might be taken to mean "Show in the options menu for search" - where we currently are - and it then responds by greying out.
I don't think this label makes it perfectly clear what disabling it really does (Stops it from showing up in the list of search engine possibilities that appear as you are typing something in the address bar).

It's certainly substantially clearer than "Enable"/"Disable" - I just wonder if we could do even better?
Attachment #777974 - Attachment is obsolete: true
Attachment #777974 - Flags: review?(liuche)
Attachment #777996 - Flags: review?(liuche)
Assignee

Comment 32

6 years ago
Attachment #777973 - Attachment is obsolete: true
Attachment #777973 - Flags: review?(liuche)
Attachment #778667 - Flags: review?(liuche)
Assignee

Comment 33

6 years ago
The uninstall option is now greyed out for bundled engines - Gecko won't allow us to actually uninstall these anyway.
Attachment #777996 - Attachment is obsolete: true
Attachment #777996 - Flags: review?(liuche)
Attachment #778702 - Flags: review?(liuche)
Comment on attachment 777996 [details] [diff] [review]
Part 1/3: New UI. V10 - Adding latest string changes.

Review of attachment 777996 [details] [diff] [review]:
-----------------------------------------------------------------

Nice job! This is looking very good (lots of nits).

In general, your code is doing the right thing, but try to be more clear about variable names and more concise/clear about comments. There are some places where variables and comments are confusing rather than as helpful as they could be.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +154,5 @@
>  <!ENTITY pref_vendor_faqs "FAQs">
>  <!ENTITY pref_vendor_feedback "Give feedback">
> +<!ENTITY pref_search_set_default "Set as default">
> +<!ENTITY pref_search_default "Default">
> +<!ENTITY pref_search_enable "Show in search options">

I think you might have a point for strings - looking at this in the UI, the context is unclear. What do you think about something like "Show (in) search results" and "Hide (from) search results"?
Nit: update the entity names to reflect the string, both here and in strings.xml.in.

@@ +156,5 @@
> +<!ENTITY pref_search_set_default "Set as default">
> +<!ENTITY pref_search_default "Default">
> +<!ENTITY pref_search_enable "Show in search options">
> +<!ENTITY pref_search_disable "Hide from search options">
> +<!ENTITY pref_search_disabled "Disabled">

I think this string is ok, but it's not very consistent with the "Show/hide" changes we've made. Give it a thought (although I'm fine with keeping it the same).

@@ +157,5 @@
> +<!ENTITY pref_search_default "Default">
> +<!ENTITY pref_search_enable "Show in search options">
> +<!ENTITY pref_search_disable "Hide from search options">
> +<!ENTITY pref_search_disabled "Disabled">
> +<!ENTITY pref_search_uninstall "Uninstall">

I find the idea of "uninstalling" a search engine still weird, so how about "Remove"?

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +21,5 @@
> +import org.mozilla.gecko.R;
> +import org.mozilla.gecko.gfx.BitmapUtils;
> +import org.mozilla.gecko.util.ThreadUtils;
> +
> +// Represents an element in the list of search engines on the preferences menu.

Use /** **/ style javadocs for comments about classes. The rest of the code is pretty lax, but let's start making new comments better!

@@ +23,5 @@
> +import org.mozilla.gecko.util.ThreadUtils;
> +
> +// Represents an element in the list of search engines on the preferences menu.
> +public class SearchEnginePreference extends Preference {
> +    // Constants representing the indices in the button array of the AlertDialog of the three buttons.

Conciseness: // Indices of AlertDialog buttons.

@@ +24,5 @@
> +
> +// Represents an element in the list of search engines on the preferences menu.
> +public class SearchEnginePreference extends Preference {
> +    // Constants representing the indices in the button array of the AlertDialog of the three buttons.
> +    public static final int SET_DEFAULT_BUTTON_POSITION = 0;

Just my preference, but I'd prefer these to all start with the same prefix. Like INDEX_SET_DEFAULT, INDEX_TOGGLE_ENABLE, INDEX_REMOVE perhaps?

@@ +28,5 @@
> +    public static final int SET_DEFAULT_BUTTON_POSITION = 0;
> +    public static final int TOGGLE_ENABLEDNESS_BUTTON_POSITION = 1;
> +    public static final int UNINSTALL_BUTTON_POSITION = 2;
> +
> +    // 'twould be nice to have this as static, but the structure just isn't there to reliably have it deallocated.

More formal style for comments.

@@ +29,5 @@
> +    public static final int TOGGLE_ENABLEDNESS_BUTTON_POSITION = 1;
> +    public static final int UNINSTALL_BUTTON_POSITION = 2;
> +
> +    // 'twould be nice to have this as static, but the structure just isn't there to reliably have it deallocated.
> +    // To avoid constantly using the resource manager, and also to avoid keeping them in memory after we stop caring.. (Static would be nice..)

same for style.

@@ +30,5 @@
> +    public static final int UNINSTALL_BUTTON_POSITION = 2;
> +
> +    // 'twould be nice to have this as static, but the structure just isn't there to reliably have it deallocated.
> +    // To avoid constantly using the resource manager, and also to avoid keeping them in memory after we stop caring.. (Static would be nice..)
> +    public String DISABLE_LABEL;

Same prefix; also, since we've been changing the strings, make sure the string names are somewhat consistent with the strings themselves.

@@ +33,5 @@
> +    // To avoid constantly using the resource manager, and also to avoid keeping them in memory after we stop caring.. (Static would be nice..)
> +    public String DISABLE_LABEL;
> +    public String ENABLE_LABEL;
> +    public String IS_DEFAULT_TEXT;
> +    // The span element used to colour text grey for disabled elements

Nit: period.

@@ +36,5 @@
> +    public String IS_DEFAULT_TEXT;
> +    // The span element used to colour text grey for disabled elements
> +    private ForegroundColorSpan disabledSpan;
> +
> +    // Does this object represent the currently configured default search engine?

I think these variables are named in an obvious way and don't really need the comments.

Also, in the future, only use declarative comments - these read like TODO/verify to me.

@@ +38,5 @@
> +    private ForegroundColorSpan disabledSpan;
> +
> +    // Does this object represent the currently configured default search engine?
> +    private boolean isDefaultEngine;
> +    // Is this engine hidden?

same

@@ +41,5 @@
> +    private boolean isDefaultEngine;
> +    // Is this engine hidden?
> +    private boolean isHiddenEngine;
> +
> +    // The strings used as labels in the prompt that appears when this item is tapped.

Nit: just "Labels for search options prompt" or something. Concise!

@@ +55,5 @@
> +    private SpannableString titleSpannable;
> +
> +    private SearchPreferenceCategory parentCategory;
> +
> +    public SearchEnginePreference(Context context, SearchPreferenceCategory backreference) {

A javadoc comment here, describing what "backreference" is. Could also just rename it to "parentCategory".

@@ +71,5 @@
> +        setOnPreferenceClickListener(new OnPreferenceClickListener() {
> +            @Override
> +            public boolean onPreferenceClick(Preference preference) {
> +                SearchEnginePreference sPref = (SearchEnginePreference) preference;
> +                //Access and display the dialog configured on the preference object.

Nit: space between // and comment.

What does "configured on the preference object" mean? Perhaps "configured from."

@@ +84,5 @@
> +        ENABLE_LABEL = res.getString(R.string.pref_search_enable);
> +        IS_DEFAULT_TEXT = res.getString(R.string.pref_search_default);
> +
> +        // Set up default dialog items.
> +        dialogItems = new String[] {res.getString(R.string.pref_search_set_default),

Nit: spaces between {} and inner contents.

@@ +92,5 @@
> +        // Spannable used for expressing enabledness.
> +        disabledSpan = new ForegroundColorSpan(res.getColor(R.color.text_color_primary_disable_only));
> +    }
> +
> +    // Configure this object to display the properties of a Gecko search engine object.

// Configure this Preference object from the Gecko search engine JSON object.

@@ +93,5 @@
> +        disabledSpan = new ForegroundColorSpan(res.getColor(R.color.text_color_primary_disable_only));
> +    }
> +
> +    // Configure this object to display the properties of a Gecko search engine object.
> +    public void setSearchEngine(JSONObject o) throws JSONException {

setSearchEngineFromJSON, perhaps? Also, name o differently, to geckoEngineJSON or something.

@@ +110,5 @@
> +            setIcon(drawable);
> +        }
> +    }
> +
> +    // Specify if this engine is the currently configured default one or not (Shows subtitle text identifying as such)

Make this more concise; nits: punctuation, capitalization.
Maybe javadoc it?
/**
    Set this engine's default state to <code>isDefault</code> and update Preference display.
...
**/

@@ +112,5 @@
> +    }
> +
> +    // Specify if this engine is the currently configured default one or not (Shows subtitle text identifying as such)
> +    public void setIsDefaultEngine(boolean isDefault) {
> +        isDefaultEngine = isDefault;

this.isDefaultEngine = isDefault to be more clear.

@@ +124,5 @@
> +            }
> +        }
> +    }
> +
> +    // Configure display for the hiddenness of this element - If hidden grey out and show "Disabled".

Nit: capitalization.

@@ +126,5 @@
> +    }
> +
> +    // Configure display for the hiddenness of this element - If hidden grey out and show "Disabled".
> +    public void setIsHiddenEngine(boolean hidden) {
> +        isHiddenEngine = hidden;

this.isHiddenEngine

@@ +128,5 @@
> +    // Configure display for the hiddenness of this element - If hidden grey out and show "Disabled".
> +    public void setIsHiddenEngine(boolean hidden) {
> +        isHiddenEngine = hidden;
> +
> +        // Update the button labels for "Enable"/"Disable" depending on enabledness state.

Update comment to match "Show/Hide".

@@ +158,5 @@
> +        // Due to Andrid's lazy loading of buttons, we can't actually configure the enabledness of the buttons until the
> +        // prompt is shown, so we do this inside the onShowListener. But first - build it!
> +
> +        // If we are the only engine left, then we are the default engine, and none of the options on this menu can do anything.
> +        if (parentCategory.isLastEngineStanding(this)) {

I'd almost rather this be made not clickable, and maybe pop up a toast explaining why, but we should see what Ian thinks about it. (I know, that makes it more complicated...)

@@ +167,5 @@
> +        builder.setTitle(getTitle().toString());
> +        builder.setItems(dialogItems, new DialogInterface.OnClickListener() {
> +            // Forward the various events that we care about to the container class for handling.
> +            @Override
> +            public void onClick(DialogInterface dialog, int which) {

indexClicked, instead of which?

@@ +172,5 @@
> +                hideDialog();
> +                switch (which) {
> +                    case SET_DEFAULT_BUTTON_POSITION:
> +                        parentCategory.setDefault(SearchEnginePreference.this);
> +                    break;

Indent: Fennec style aligns break; with previous line. Do the same for the other cases.

@@ +178,5 @@
> +                        parentCategory.toggleEnabledness(SearchEnginePreference.this);
> +                    break;
> +                    case UNINSTALL_BUTTON_POSITION:
> +                        parentCategory.uninstall(SearchEnginePreference.this);
> +                    break;

Add a default: with a Log.w.

@@ +181,5 @@
> +                        parentCategory.uninstall(SearchEnginePreference.this);
> +                    break;
> +                }
> +            }
> +        });

Nit: newline.

@@ +193,5 @@
> +            @Override
> +            public void run() {
> +                dialog = builder.create();
> +                dialog.setOnShowListener(new DialogInterface.OnShowListener() {
> +                    // Called when the dialog is shown (And we're finally able to manipulate button enabledness)

Nit: period.

@@ +217,5 @@
> +        });
> +    }
> +
> +    // Having shown the dialog, the lazily-created button elements therein will have been spawned by Android, so we can
> +    // now muck about with them as we want.

More formal comment style.

@@ +219,5 @@
> +
> +    // Having shown the dialog, the lazily-created button elements therein will have been spawned by Android, so we can
> +    // now muck about with them as we want.
> +    private void configureShownDialog() {
> +        // If we are the default engine, disable the "Set default" button.

Update comment to reflect strings.

@@ +221,5 @@
> +    // now muck about with them as we want.
> +    private void configureShownDialog() {
> +        // If we are the default engine, disable the "Set default" button.
> +        TextView defaultButton = (TextView) dialog.getListView().getChildAt(SET_DEFAULT_BUTTON_POSITION);
> +        // Disable "Set default" button if we are already the default.

same

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +1,1 @@
>  package org.mozilla.gecko.preferences;

Add a license here.

@@ +22,1 @@
>      }

Nit: while you're here, add a newline.

@@ +31,5 @@
>      @Override
>      protected void onAttachedToActivity() {
>          super.onAttachedToActivity();
>  
> +        // Useful for ensuring the default engine is always at the top of the list.

"Ensure default engine is at top of list."

@@ +44,5 @@
>      public void handleMessage(String event, final JSONObject data) {
>          if (event.equals("SearchEngines:Data")) {
>              JSONArray engines;
> +            JSONObject defaultEngine;
> +            final String defaultEngineName;

Comment here:
// Get engines array from JSON, and also fetch the default engine name from first item in array.

or something along those lines.

@@ +49,3 @@
>              try {
>                  engines = data.getJSONArray("searchEngines");
> +                if (engines.length() == 0) {

Should there ever be no engines? Perhaps Log.w here, "No search engines."

@@ +63,5 @@
>                  try {
>                      JSONObject engineJSON = engines.getJSONObject(i);
>                      final String engineName = engineJSON.getString("name");
>  
> +                    SearchEnginePreference engine = new SearchEnginePreference(getContext(), this);

Lots of "engine" in this section here - let's be more explicit and name this enginePreference.

@@ +64,5 @@
>                      JSONObject engineJSON = engines.getJSONObject(i);
>                      final String engineName = engineJSON.getString("name");
>  
> +                    SearchEnginePreference engine = new SearchEnginePreference(getContext(), this);
> +                    engine.setSearchEngine(engineJSON);

I'd prefer this to be named explicitly too - setSearchEnginFromJSON or something.

@@ +68,5 @@
> +                    engine.setSearchEngine(engineJSON);
> +                    if (engineName.equals(defaultEngineName)) {
> +                        engine.setIsDefaultEngine(true);
> +                        defaultEngineReference = engine;
> +                    }

Nit: newline.

@@ +73,5 @@
> +                    engine.setOnPreferenceClickListener(new OnPreferenceClickListener() {
> +                        @Override
> +                        public boolean onPreferenceClick(Preference preference) {
> +                            SearchEnginePreference sPref = (SearchEnginePreference) preference;
> +                            //Access and display the dialog configured on the preference object.

Nit: space between // and comment.
Also, "configured on the preference object"? Can you make that a little more clear?

@@ +88,5 @@
>          }
> +        GeckoAppShell.unregisterEventListener("SearchEngines:Data", this);
> +    }
> +
> +    boolean isLastEngineStanding(SearchEnginePreference engine) {

More formal method name: isLastEnabledEngine

@@ +91,5 @@
> +
> +    boolean isLastEngineStanding(SearchEnginePreference engine) {
> +        if (engine.getIsHiddenEngine()) {
> +            return false;
> +        }

Nit: newline.

@@ +92,5 @@
> +    boolean isLastEngineStanding(SearchEnginePreference engine) {
> +        if (engine.getIsHiddenEngine()) {
> +            return false;
> +        }
> +        int prefs = getPreferenceCount();

Nit: numPrefs.

@@ +94,5 @@
> +            return false;
> +        }
> +        int prefs = getPreferenceCount();
> +        for (int i = 0; i < prefs; i++) {
> +            SearchEnginePreference thisEngine = (SearchEnginePreference) getPreference(i);

thisEngine is confusing - either the "engine" argument should be "thisEngine," or "thisEngine" should be "aEngine".

@@ +104,5 @@
> +    }
> +
> +    private void setFallbackDefaultEngine() {
> +        // Set the default engine to an arbitrary available engine. Useful if the default is removed or disabled.
> +        int prefs = getPreferenceCount();

Nit: numPrefs.

@@ +106,5 @@
> +    private void setFallbackDefaultEngine() {
> +        // Set the default engine to an arbitrary available engine. Useful if the default is removed or disabled.
> +        int prefs = getPreferenceCount();
> +        for (int i = 0; i < prefs; i++) {
> +            SearchEnginePreference thisEngine = (SearchEnginePreference) getPreference(i);

same: use aEngine.

@@ +116,5 @@
> +    }
> +
> +    // Methods called by tapping items on the submenus for each search engine.
> +    public void toggleEnabledness(SearchEnginePreference engine) {
> +        boolean isHidden = !engine.getIsHiddenEngine();

I understand what you're doing here, but as it's written, this is a confusing line because it's taking "isHidden" and setting it to NOT "getIsHiddenEngine()". You should rename isHidden to something like "newHiddenState" so that line doesn't look like a blatant bug.

@@ +128,5 @@
> +
> +    public void uninstall(SearchEnginePreference engine) {
> +        removePreference(engine);
> +        if (engine == defaultEngineReference) {
> +            // If they're deleting their default engine, get them a new default engine..

Nit: remove double ..

::: mobile/android/chrome/content/browser.js
@@ +6373,5 @@
>        suggestEngine = engine.name;
>        suggestTemplate = engine.getSubmission("__searchTerms__", "application/x-suggestions+json").uri.spec;
>      }
>  
> +    // By convention, the currently configured default engine is at position zero in searchEngines

Nit: period.
Attachment #777996 - Attachment is obsolete: false
Assignee

Comment 35

6 years ago
> ::: mobile/android/base/preferences/SearchEnginePreference.java
> @@ +21,5 @@
> > +import org.mozilla.gecko.R;
> > +import org.mozilla.gecko.gfx.BitmapUtils;
> > +import org.mozilla.gecko.util.ThreadUtils;
> > +
> > +// Represents an element in the list of search engines on the preferences menu.
> 
> Use /** **/ style javadocs for comments about classes. The rest of the code
> is pretty lax, but let's start making new comments better!

Ah! I had no idea Javadoc comments were desired. Javadoc comments will be provided.

> @@ +158,5 @@
> > +        // Due to Andrid's lazy loading of buttons, we can't actually configure the enabledness of the buttons until the
> > +        // prompt is shown, so we do this inside the onShowListener. But first - build it!
> > +
> > +        // If we are the only engine left, then we are the default engine, and none of the options on this menu can do anything.
> > +        if (parentCategory.isLastEngineStanding(this)) {
> 
> I'd almost rather this be made not clickable, and maybe pop up a toast
> explaining why, but we should see what Ian thinks about it. (I know, that
> makes it more complicated...)

Toasts added - Good idea.

> @@ +49,3 @@
> >              try {
> >                  engines = data.getJSONArray("searchEngines");
> > +                if (engines.length() == 0) {
> 
> Should there ever be no engines? Perhaps Log.w here, "No search engines."

You can't ever have no engines - Gecko won't allow you to delete the bundled engines.
Attachment #777996 - Attachment is obsolete: true
Attachment #778702 - Attachment is obsolete: true
Attachment #778702 - Flags: review?(liuche)
Attachment #778770 - Flags: review?(liuche)
Assignee

Comment 36

6 years ago
Attachment #778667 - Attachment is obsolete: true
Attachment #778667 - Flags: review?(liuche)
Attachment #778771 - Flags: review?(liuche)
Assignee

Updated

6 years ago
tracking-fennec: --- → ?
Assignee

Updated

6 years ago
See Also: → 898151
Assignee

Updated

6 years ago
Status: NEW → ASSIGNED
Comment on attachment 778770 [details] [diff] [review]
Part 1/3: New UI. V12 - Nits on toast.

Review of attachment 778770 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry this took so long! This is looking great. Some nits, and if you could roll a build and link it so Ian can see the strings (needinfo him), we can get land this.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +158,5 @@
> +<!ENTITY pref_search_shortcuts_enable "Show in search shortcuts">
> +<!ENTITY pref_search_shortcuts_disable "Hide from search shortcuts">
> +<!ENTITY pref_search_disabled "Disabled">
> +<!ENTITY pref_search_remove "Remove">
> +<!ENTITY pref_search_last_toast "You can\'t remove or disable your last search engine.">

Let's check with Ian about this string. Can you roll a build for him to test this out?

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +41,5 @@
> +
> +    // Cache labels to avoid repeated use of the resource system.
> +    public String LABEL_SHORTCUTS_HIDE;
> +    public String LABEL_SHORTCUTS_SHOW;
> +    public String LABEL_IS_DEFAULT;

nit: newline

@@ +50,5 @@
> +    private boolean mIsDisabledEngine;
> +    private boolean mIsImmutableEngine;
> +
> +    // Dialog element labels.
> +    private String[] mDialogItems;

nit: remove newline

@@ +140,5 @@
> +        }
> +    }
> +
> +    /**
> +     * Set if this object's UI should show that this engine is disabled (Disabled)

Nit: "Disabled" in quotes

@@ +159,5 @@
> +        } else {
> +            mDialogItems[INDEX_ENABLEDNESS_BUTTON] = LABEL_SHORTCUTS_HIDE;
> +            // Reset colour of the title of this search engine.
> +            titleSpannable.removeSpan(disabledSpan);
> +            // In the almost-always true event that we're not a disabled default engine...

This does not apply anymore, right? Default engines can no longer be disabled.

@@ +207,5 @@
> +                    case INDEX_UNINSTALL_BUTTON:
> +                        mParentCategory.uninstall(SearchEnginePreference.this);
> +                        break;
> +                    default:
> +                        Log.wtf(LOGTAG, "Selected index out of range.");

Heh, as tempting as it is to use Log.wtf is, just use Log.warn please.

@@ +224,5 @@
> +            @Override
> +            public void run() {
> +                mDialog = builder.create();
> +                mDialog.setOnShowListener(new DialogInterface.OnShowListener() {
> +                    // Called when the dialog is shown (And we're finally able to manipulate button enabledness).

Nit: lowercase the "And"

@@ +263,5 @@
> +        if (mIsDefaultEngine) {
> +            defaultButton.setEnabled(false);
> +            // Failure to unregister this listener leads to tapping the button dismissing the dialog without doing anything.
> +            defaultButton.setOnClickListener(null);
> +        }

Nit: newline

@@ +265,5 @@
> +            // Failure to unregister this listener leads to tapping the button dismissing the dialog without doing anything.
> +            defaultButton.setOnClickListener(null);
> +        }
> +        if (mIsImmutableEngine) {
> +            // Prevent the uninstallation of bundled engines (Gecko forbids this).

Nit: move this comment one line up.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +143,5 @@
> +     * Called from {@link org.mozilla.gecko.preferences.SearchEnginePreference}.
> +     * @param engine The engine to have enabledness toggled.
> +     */
> +    public void toggleEnabledness(SearchEnginePreference engine) {
> +        // Our new disabled state...

Nit: comment is unnecessary, as you've renamed the variable better.

::: mobile/android/chrome/content/browser.js
@@ +6364,3 @@
>      let searchEngines = engineData.map(function (engine) {
> +      let isImmutable = false;
> +      for (let i = 0; i < immutableEngines.length; i++) {

Can you use indexOf instead?

@@ +6410,2 @@
>    observe: function observe(aSubject, aTopic, aData) {
> +    if (aTopic == "SearchEngines:GetVisible") {

My one concern is the tradeoff between more message observers vs passing/parsing JSON. These patches definitely go in the "more messages" direction - I guess for now at least, since I don't know the relatibe cost of either, that should be fine.
Attachment #778770 - Flags: review?(liuche) → review+
Comment on attachment 778771 [details] [diff] [review]
Part 2/3: Communication with Gecko to save the changes made via the UI. V4 - Further nit removal

Review of attachment 778771 [details] [diff] [review]:
-----------------------------------------------------------------

Heh, much shorter patch. Two nits, then r+.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +140,5 @@
> +     * Helper method to send a particular event string to Gecko with an associated engine name.
> +     * @param engine The engine to which the event relates.
> +     * @param event The type of event to send.
> +     */
> +    private void sendGeckoEngineEvent(SearchEnginePreference engine, String event) {

Nit: event as arg 1, and engine as arg 2. Stylistically, this seems more consistent with other event calls, and it makes more sense to send it as method: args form.

::: mobile/android/chrome/content/browser.js
@@ +6429,5 @@
> +      break;
> +      case "SearchEngines:Get":
> +        // Return a list of all engines, including "Hidden" ones.
> +        Services.search.init(this._handleSearchEnginesGetAll.bind(this));
> +      break;

Nit: align break with content lines, here and below.
Attachment #778771 - Flags: review?(liuche) → review+
Comment on attachment 776228 [details] [diff] [review]
Part 3/3: Eliminate the original method of changing search engine settings (Via the addons menu)

Review of attachment 776228 [details] [diff] [review]:
-----------------------------------------------------------------

Remove the unused strings - I think these are in locale/en-US/chrome/, and make sure there aren't others.
Attachment #776228 - Flags: review?(liuche) → review-
Finally, push these patches to try and make sure there aren't any broken tests - or fix them if there are.
Assignee

Updated

6 years ago
Attachment #776228 - Attachment is obsolete: true
Wonderful, the try push is green, so once you get some final patches up, we can land this and let it sit on nightly for a few days before merge.
Oh, just noticed that you already have the cleaned up patches up.

Can you make a build for Ian to run, and needinfo him for that? We just need to run these changes (string, etc) by him.

Also include in the needinfo comment a list of the UI changes that need some looking over (strings, removing all the engines + toast, etc).
Flags: needinfo?(ckitching)
Assignee

Comment 47

6 years ago
Ian, here's a build with the current patches applied for your perusal:
https://www.dropbox.com/s/q5e9s0ajpd6ir94/fennec-25.0a1.en-US.android-arm.apk

What's your opinion on the changes we made to the menu labels? (The "Show/hide from search shortcuts" thing).
Do you like what currently happens when you go to remove or disable your last remaining search engine? (It shows a toast and tells you this isn't allowed)
Any other thoughts?
Flags: needinfo?(ckitching) → needinfo?(ibarlow)
Overall, the new strings look good to me.

However, I don't get why "Remove" is greyed out in all of these options... I see that if I add another engine like Duckduckgo I can remove that one though. Makes for pretty confusing experience.

Are we trying to solve for the case of what happens when you remove all your search engines? Maybe we can be smarter about it than not allowing people to change things in an inconsistent way.

Perhaps if I remove all my pre-installed search engines a row appears in settings that reads "Reinstall default search engines". Tapping it opens a context menu with checkboxes:


        +--------------------------------------+
        |                                      |
        | Reinstall default search engines     |
        |                                      |
        +--------------------------------------+
        |+-+                                   |
        ||X|  Google                           |
        |+-+                                   |
        +--------------------------------------+
        |+-+                                   |
        ||X|  Amazon                           |
        |+-+                                   |
        +--------------------------------------+
        |+-+                                   |
        ||X|  Twitter                          |
        |+-+                                   |
        +--------------------------------------+
        |+-+                                   |
        ||X|  Wikipedia                        |
        |+-+                                   |
        +------------------+-------------------+
        |                  |                   |
        |     Cancel       |        OK         |
        |                  |                   |
        +------------------v-------------------+
Flags: needinfo?(ibarlow)
I'm pretty sure "Remove" is disabled because it is not possible to remove a built-in search provider. We should probably not even show the "Remove" menu for those.

IMO, "Show/Hide from search shortcuts" is confusing and will translate (l10n) poorly. I'd be happier with the old "Enable/Disable" strings.
Comment on attachment 782347 [details] [diff] [review]
Part 3/3: Eliminate the original method of changing search engine settings (Via the addons menu) V2 - Removing redundant strings.

Review of attachment 782347 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r+ - sorry for taking so long!
Attachment #782347 - Flags: review?(liuche) → review+
Assignee

Comment 51

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #49)
> I'm pretty sure "Remove" is disabled because it is not possible to remove a
> built-in search provider. We should probably not even show the "Remove" menu
> for those.
> 
> IMO, "Show/Hide from search shortcuts" is confusing and will translate
> (l10n) poorly. I'd be happier with the old "Enable/Disable" strings.

I also think "Enable/Disable" are simpler, but they don't make it entirely clear what this actually DOES. What does it mean to "Disable" Google? Disabling Google sounds like a particularly odd thing to do. Does this mean I can't use Google any more? This creates its own flavour of confusion, just perhaps one that we aren't susceptible to.

You are also correct about why "Remove" is disabled - Gecko does not allow removal of bundled search engines, period. I guess there's some licensing rule behind this or something, but that's the way it is. You can of course remove engines you add yourself. It would indeed probably be less confusing to not show the "Remove" option here.

On the topic of the "Reinstall default search engines" thing - if that were implemented, it would once again be possible to disable/remove all search engines. In this situation, when you type something that isn't a URL that can be navigated to into the AwesomeBar you get an error page, instead of it doing anything more useful.
Isn't it the case that the sort of person who would do this is probably exactly the sort of person we want to keep from doing this, because they're doing it by mistake because they're confused. (I doubt the existence of people who genuinely want to be able to do this).


So, given the shortness of time, we need to come to a decision about:

Strings: Keep them as they are, or revert to "Enable/Disable"?
Removal: Delete the "Remove" option from the menu for all bundled engines?
Removal: Do we really want the ability to pseudo-uninstall all bundled engines as Ian suggests, enabling you to run Fennec with no search providers at all?

Sort of nowish.
We started talking about this on IRC. I notice that desktop does not have "enable/disable" actions for Search Engines. Only "Remove" and reordering is supported.

I like removing the "Enable/Disable" search engine actions from Fennec. That leaves "Remove". Removing engines that I installed seems safe. To add the engine back I need to re-install it. Removing built-in engines means "hiding" them since we can't really remove them. No big deal, but how to we get them back? Desktop uses a "Restore defaults" button. We could add that too, but I think we start to push beyond what we can get done in 3 days.

Chrome on Android does not allow the user to remove built-in search engines. I could live with that compromise for Fx25, maybe longer.

I propose:
* Remove the "Enable/Disable" actions from all engines
* Only show a "Remove" action if it's a search engine the user installed.

Thoughts?
Flags: needinfo?(ibarlow)
Assignee

Comment 53

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #52)
> We started talking about this on IRC. I notice that desktop does not have
> "enable/disable" actions for Search Engines. Only "Remove" and reordering is
> supported.
> 
> I like removing the "Enable/Disable" search engine actions from Fennec. That
> leaves "Remove". Removing engines that I installed seems safe. To add the
> engine back I need to re-install it. Removing built-in engines means
> "hiding" them since we can't really remove them. No big deal, but how to we
> get them back? Desktop uses a "Restore defaults" button. We could add that
> too, but I think we start to push beyond what we can get done in 3 days.
> 
> Chrome on Android does not allow the user to remove built-in search engines.
> I could live with that compromise for Fx25, maybe longer.
> 
> I propose:
> * Remove the "Enable/Disable" actions from all engines
> * Only show a "Remove" action if it's a search engine the user installed.
> 
> Thoughts?

This sounds distinctly doable - the only objection I can think of is that we might want to have the "hide" feature, since re-adding engines on mobile is more fiddly than on the desktop. (A sort of plausible use-case is I might want to try out a new search engine before ultimately returning to my earlier one - I'd rather not have to uninstall and reinstall the original engine when doing this.)

This suggestion now makes it once again possible to remove every search engine. In this state, Fennec gives you an error page if you input a non-URL into the address bar.
Are you sure you want that to be possible?
Or did you want to keep the toast that prevents you from deleting the last one left?
Assignee

Comment 54

6 years ago
I'm sorry, I wasn't clear - my above comment refers to your upper, more ideal solution. I was not commenting on your compromise solution (Which also sounds entirely straightforward)
Assignee

Comment 55

6 years ago
So since there seems to be uncertainty about where we're going with this, I've taken the liberty of implementing some patches-for-the-patches on this topic. They fit between parts 2 and 3 already presented.
When a final decision is made I shall fold the appropriate patches. I suspect this approach makes the lives of the reviewers better in the meantime (And makes it easier for people to muck about with patches and try out different approaches).

This post: Patch removing the "Remove" button from bundled engines.
Assignee

Comment 56

6 years ago
Patch removing the "Enable/Disable" buttons entirely.
Assignee

Comment 57

6 years ago
Patch, stacked on the prior two foldables, which replaces the ability to delete bundled search engines. When all bundled engines are deleted, a special element appears which, when tapped, restores the bundled engines.

So, for Finkle's proposed compromise we fold the first two patches. For the extended can-also-delete-defaults solution we fold all three.

Builds of these to follow for possible perusal by Ian.
Assignee

Comment 58

6 years ago
Build with all foldables:
https://www.dropbox.com/s/9q2rjm2dtu0ztc8/allFolded.apk

Build with the first two foldables:
https://www.dropbox.com/s/gdr9itam04mfsp3/twoFolded.apk

Build with just the first foldable:
https://www.dropbox.com/s/yiaxwnqoil049sd/oneFolded.apk

If we go for the approach of being-allowed-to-delete-bundled-engines-and-then-restore-them-with-a-button, is the implementation provided sufficient, or shall I enhance it with the checkbox as Ian suggested above?
What if I delete some, but not all of the default providers? Should there be a way of getting them back without having to counter-intuitively delete the remainder to enable the special entry?
How do we deal with the no-search-engines-exist state which would then become possible?


Personally, I preferred the implementation as it was before this latest round of changes. I liked the ability to toggle engines on/off without removing them from the menu. Not being able to remove the bundled ones was not a bother to me - the ideal combination for me would probably be just to hide the "Remove" button from the bundled  ones and be done with it. But then, I'm strange. :P
Ian and I chatted on IRC. For now, we want to go with the minimum proposal in comment 52:
* Remove the "Enable/Disable" actions from all engines
* Only show a "Remove" action if it's a search engine the user installed.

I think that means we want to review attachment 783628 [details] [diff] [review] and attachment 783629 [details] [diff] [review]. We can evaluate adding the additional functionality later, but these two attachments should be enough to safely ship this feature in Fx25.
Flags: needinfo?(ibarlow)
Reporter

Comment 60

6 years ago
(In reply to Chris Kitching [:ckitching] from comment #53)
> (In reply to Mark Finkle (:mfinkle) from comment #52)
> > We started talking about this on IRC. I notice that desktop does not have
> > "enable/disable" actions for Search Engines. Only "Remove" and reordering is
> > supported.
> > 
> > I like removing the "Enable/Disable" search engine actions from Fennec. That
> > leaves "Remove". Removing engines that I installed seems safe. To add the
> > engine back I need to re-install it. Removing built-in engines means
> > "hiding" them since we can't really remove them. No big deal, but how to we
> > get them back? Desktop uses a "Restore defaults" button. We could add that
> > too, but I think we start to push beyond what we can get done in 3 days.
> > 
> > Chrome on Android does not allow the user to remove built-in search engines.
> > I could live with that compromise for Fx25, maybe longer.
> > 
> > I propose:
> > * Remove the "Enable/Disable" actions from all engines
> > * Only show a "Remove" action if it's a search engine the user installed.
> > 
> > Thoughts?
> 
> This sounds distinctly doable - the only objection I can think of is that we
> might want to have the "hide" feature, since re-adding engines on mobile is
> more fiddly than on the desktop. (A sort of plausible use-case is I might
> want to try out a new search engine before ultimately returning to my
> earlier one - I'd rather not have to uninstall and reinstall the original
> engine when doing this.)

You can do this by just choosing to do the search with that engine sometimes. Having an extra engine installed is pretty unobtrusive. I feel like we just have those enable/disable options in the current implementation because that's what we have for add-ons (and search engines are in the add-ons manager), so we shouldn't feel tied to that.
tracking-fennec: ? → 25+
Assignee

Comment 61

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #59)
> Ian and I chatted on IRC. For now, we want to go with the minimum proposal
> in comment 52:
> * Remove the "Enable/Disable" actions from all engines
> * Only show a "Remove" action if it's a search engine the user installed.
> 
> I think that means we want to review attachment 783628 [details] [diff] [review]
> [review] and attachment 783629 [details] [diff] [review]. We can evaluate
> adding the additional functionality later, but these two attachments should
> be enough to safely ship this feature in Fx25.

Okay. This means the only item on the menu for bundled search engines will be "Set as default". Do we still want to prompt the user with a one-item popup menu in this case, or should it set as default as soon as you tap? (Creating an inconsistency with engines you've added yourself)
Reporter

Comment 62

6 years ago
(In reply to Chris Kitching [:ckitching] from comment #61)
> (In reply to Mark Finkle (:mfinkle) from comment #59)
> > Ian and I chatted on IRC. For now, we want to go with the minimum proposal
> > in comment 52:
> > * Remove the "Enable/Disable" actions from all engines
> > * Only show a "Remove" action if it's a search engine the user installed.
> > 
> > I think that means we want to review attachment 783628 [details] [diff] [review]
> > [review] and attachment 783629 [details] [diff] [review]. We can evaluate
> > adding the additional functionality later, but these two attachments should
> > be enough to safely ship this feature in Fx25.
> 
> Okay. This means the only item on the menu for bundled search engines will
> be "Set as default". Do we still want to prompt the user with a one-item
> popup menu in this case, or should it set as default as soon as you tap?
> (Creating an inconsistency with engines you've added yourself)

I think this sounds fine, especially for a first version. At this point, I don't think we have the time to think about a different interaction pattern for this case.
Assignee

Comment 63

6 years ago
Okay then, generated new versions of the two affected patches here...
Attachment #783628 - Attachment is obsolete: true
Attachment #783629 - Attachment is obsolete: true
Attachment #783631 - Attachment is obsolete: true
Attachment #784730 - Flags: review?(liuche)
Assignee

Updated

6 years ago
Attachment #782345 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #784730 - Attachment description: New UI. V14 - Remove enable/disable functionality, remove disable button from bundled engines. → Part 1/3: New UI. V14 - Remove enable/disable functionality, remove disable button from bundled engines.
Assignee

Updated

6 years ago
Attachment #782346 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #784732 - Flags: review?(liuche)
(In reply to :Margaret Leibovic from comment #62)

> > Okay. This means the only item on the menu for bundled search engines will
> > be "Set as default". Do we still want to prompt the user with a one-item
> > popup menu in this case, or should it set as default as soon as you tap?
> > (Creating an inconsistency with engines you've added yourself)
> 
> I think this sounds fine, especially for a first version. At this point, I
> don't think we have the time to think about a different interaction pattern
> for this case.

Right. It's fine if there is only "Set as default" on the prompt for built-in engines.
Assignee

Updated

6 years ago
Blocks: 898151
Comment on attachment 784730 [details] [diff] [review]
Part 1/3: New UI. V14 - Remove enable/disable functionality, remove disable button from bundled engines.

Review of attachment 784730 [details] [diff] [review]:
-----------------------------------------------------------------

A few more nits, and then should be good.

Also of note, versioning of the patches was very helpful! I learned about the nifty interdiff feature, which lets me diff between two versions of patches.

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +33,5 @@
> +    public static int sIconSize;
> +
> +    // Indices in button array of the AlertDialog of the three buttons.
> +    public static final int INDEX_SET_DEFAULT_BUTTON = 0;
> +    public static final int INDEX_UNINSTALL_BUTTON = 1;

Possibly update this to REMOVE.

@@ +40,5 @@
> +    public String LABEL_IS_DEFAULT;
> +
> +    // Specifies if this engine is configured as the default search engine.
> +    private boolean mIsDefaultEngine;
> +    // Specifies if this engine is one of the ones bundled with the app, which cannot delete.

"which cannot be deleted"

@@ +95,5 @@
> +        SpannableString titleSpannable = new SpannableString(engineName);
> +        mIsImmutableEngine = geckoEngineJSON.getBoolean("immutable");
> +
> +        if (mIsImmutableEngine) {
> +            // Delete the "Remove" option from the menu...

Nit: remove ellipsis.

@@ +146,5 @@
> +            });
> +            return;
> +        }
> +
> +        // If there are no enabled items on the menu, abort.

I don't think this comment reflects what you are doing. Rather, something about default-immutable engines having no actions.

Is there a way to avoid displaying the click highlight action? We could also file this as a follow-up, or handle it in the next iteration.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +115,5 @@
> +            setDefault(aEngine);
> +        }
> +    }
> +
> +    // Methods called by tapping items on the submenus for each search engine...

Nit: Remove ellipsis.

::: mobile/android/chrome/content/browser.js
@@ +6457,5 @@
>        return {
>          name: engine.name,
>          identifier: engine.identifier,
> +        iconURI: (engine.iconURI ? engine.iconURI.spec : null),
> +        hidden: engine.hidden,

Is it possible to hide engines anymore? Are they not just removed?
Attachment #784730 - Flags: review?(liuche) → review-
Comment on attachment 784732 [details] [diff] [review]
Part 2/3: Communication with Gecko to save the changes made via the UI. V6 - Removing enable/disable support.

Review of attachment 784732 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there. Also, make sure that all the patches apply on top of each other. I think Part 3 needs a trivial unbitrot to apply.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +125,5 @@
> +        JSONObject json = new JSONObject();
> +        try {
> +            json.put("engine", engine.getTitle());
> +        } catch (JSONException e) {
> +            Log.e(LOGTAG, "JSONException creating enabledness toggle message for Gecko.", e);

Is enabledness still relevant? Or maybe you mean a different word?

::: mobile/android/chrome/content/browser.js
@@ +6420,5 @@
>  
>    init: function init() {
>      Services.obs.addObserver(this, "SearchEngines:Get", false);
>      Services.obs.addObserver(this, "SearchEngines:GetVisible", false);
> +    Services.obs.addObserver(this, "SearchEngines:Enable", false);

Doesn't this series of patches get rid of Enable/Disable?

@@ +6423,5 @@
>      Services.obs.addObserver(this, "SearchEngines:GetVisible", false);
> +    Services.obs.addObserver(this, "SearchEngines:Enable", false);
> +    Services.obs.addObserver(this, "SearchEngines:Disable", false);
> +    Services.obs.addObserver(this, "SearchEngines:SetDefault", false);
> +    Services.obs.addObserver(this, "SearchEngines:Uninstall", false);

Whichever you decide (remove, uninstall) for Part 1, make sure this is consistent.

@@ +6510,5 @@
> +    let engine;
> +    switch(aTopic) {
> +      case "SearchEngines:GetVisible":
> +        Services.search.init(this._handleSearchEnginesGetVisible.bind(this));
> +      break;

Style: break should be aligned with case content. Perhaps update your IDE style rules?

@@ +6515,5 @@
> +      case "SearchEngines:Get":
> +        // Return a list of all engines, including "Hidden" ones.
> +        Services.search.init(this._handleSearchEnginesGetAll.bind(this));
> +        break;
> +      case "SearchEngines:Enable":

Same as above: do we still need enable/disable?

@@ +6537,5 @@
> +        engine.hidden = false;
> +        Services.search.removeEngine(engine);
> +        break;
> +      default:
> +        dump("Unexpected message type observed: "+aTopic);

Nit: spaces around + operator.
Attachment #784732 - Flags: review?(liuche) → review-
Assignee

Comment 68

6 years ago
(In reply to Chenxia Liu [:liuche] from comment #66)
> Is there a way to avoid displaying the click highlight action? We could also
> file this as a follow-up, or handle it in the next iteration.

Yes, but this would require reordering this collection of related bugs - a followup after landing this chunk of stuff would be considerably easier (And a miniscule patch to review etc.)

> ::: mobile/android/chrome/content/browser.js
> @@ +6457,5 @@
> >        return {
> >          name: engine.name,
> >          identifier: engine.identifier,
> > +        iconURI: (engine.iconURI ? engine.iconURI.spec : null),
> > +        hidden: engine.hidden,
> 
> Is it possible to hide engines anymore? Are they not just removed?

This one is sort of annoying. When we tell Gecko to remove a bundled (immutable) engine it responds by hiding it for us. I chose to make the real picture of what's in Gecko's data structures available to Java, should we ever want to rejig the frontend.
(Notably, if we ever want to be able to remove the default engines and then restore them later then sending all the engines is necessary for the restoration button to work. I think.)


Fifteenth time lucky, perhaps?
Attachment #784730 - Attachment is obsolete: true
Attachment #785183 - Flags: review?(liuche)
Assignee

Updated

6 years ago
Attachment #785183 - Attachment description: New UI. V15 - Nits and nomenclature. → Part 1/3: New UI. V15 - Nits and nomenclature.
Comment on attachment 785183 [details] [diff] [review]
Part 1/3: New UI. V15 - Nits and nomenclature.

Review of attachment 785183 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Done with this patch! (Be sure to file the follow-ups, please.)
Attachment #785183 - Flags: review?(liuche) → review+
Assignee

Comment 70

6 years ago
Well spotted! I left all the dead JS code behind.
Attachment #784732 - Attachment is obsolete: true
Attachment #785197 - Flags: review?(liuche)
Comment on attachment 785197 [details] [diff] [review]
Part 2/3: Communication with Gecko to save the changes made via the UI. V7 - Removing dead code. Nits.

Review of attachment 785197 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Attachment #785197 - Flags: review?(liuche) → review+
Assignee

Updated

6 years ago
Attachment #782347 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: checkin to fx-team
Assignee

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: checkin to fx-team
Assignee

Comment 73

6 years ago
Attachment #785183 - Attachment is obsolete: true
Landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/aa054b0f118b
Target Milestone: --- → Firefox 25
Backed out for turning rc2 tests permanently orange:

https://hg.mozilla.org/integration/fx-team/rev/50247b78a831
Assignee

Comment 78

6 years ago
This is, in retrospect, not surprising - the testAddSearchEngine test is completely wrong for this new search engine management system, so clearly it would fail.

It is, however, very surprising that this:
https://tbpl.mozilla.org/?tree=Try&rev=327ab519bf32
was all green. It was not the newest version of the patch, but it included the critical changes that I would have expected would cause this test to fail.. (Namely, deleting all the old search engine management code)

Is this an "Intermittent green"?

It would be nice if I could manage to actually land something without it exploding shortly thereafter at least once during my internship.

Time for a robocop test patch, then. Onward!
Assignee

Comment 79

6 years ago
Since the problem here seems to be the test's fault, I'm seperating the string changes from this patch set into its own patch which we can hopefully land in time to not miss the boat on internationalisation. (I guess this is the right procedure for this scenario?)
It is my hope that the remainder can be uplifted in due course. It would be irritating to have to postpone this until FF 26.
Attachment #785489 - Flags: review?(liuche)
Assignee

Comment 80

6 years ago
Attachment #785298 - Attachment is obsolete: true
Unfortunately, it doesn't quite work this way - in order to localize a string, localizers must be able to see the string it UI to understand its context.

Chris, there seem to be a couple of outcomes:
1) If you can fix the test to handle the search settings change correctly, please push to try as soon as you get a fix and I can review and land the patches tomorrow.

2) If for some reason, you can't fix the test, you should at least update the patches for the other two bugs so that those can land - the "search engine tip" bug might be slightly dependent on this one (you probably should be able to delete a search engine you added), but the icon size one is definitely independent. Perhaps also add a patch to disable clicks on the existing search engines list, so there isn't confusing UI (e.g., clicking registers but does nothing).

3) If all else fails, no worries - we'll definitely get this into 26!
Assignee

Comment 82

6 years ago
I see. Thanks for the info about the localisation procedures.

I believe I have now found the problem - this test uses a slightly odd way of parsing JSON. This caused my addition of an extra field to the JSON objects sent to Java from Gecko listing the search engines to lead to the test ending up with a nonexistent field called "true,name" (It would combine the value of the previous field with the name of this field)
Subsequently, it would count how many "name" fields it finds, and use this to tell how many search engines there were. It got upset when the values before and after adding an engine were the same (Zero, that is).

I'll have a patch through try by the time sane people are awake. I appreciate your help in landing this.
Assignee

Updated

6 years ago
Attachment #785489 - Attachment is obsolete: true
Attachment #785489 - Flags: review?(liuche)
Assignee

Updated

6 years ago
Attachment #785490 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #785298 - Attachment is obsolete: false
Assignee

Comment 83

6 years ago
This appears to have corrected the problem - it is no longer reproducible locally, and a try push is underway at:
https://tbpl.mozilla.org/?tree=Try&rev=a558b4fbb728
Provided that all goes green we're in business.

The problem was caused by BaseTest.parseEventData - a limited JSON parser. This method is only used by the search engine test, is unnecessary, and is fundamentally flawed in implementation - so I'm removing it with this patch (As far as I can tell there's nothing this function can do that org.json.* couldn't do better, so to prevent some future test-writer from using this dodgy one it seems wise to get rid of it)
The fix is just to replace the use of this method with the standard JSON parsing libraries.

You may notice that the getSearchEngineNames method is used exclusively to count the engines at this time - this was the structure of the original implementation, and I think leaving it this way makes it much more obvious how to extend to test to cover more of the test system. (It'd be a pretty small job to expand it to cover all I recently added - followup bug, perhaps?)

The code in this test was extremely messy. I'm also including a supplemental patch to make it less confusing - review and include it if you want - it's not necessary, but since I wrote extra comments to get my head round the code it seems possibly useful to submit a cleaned-up edition for future benefit.
Attachment #785500 - Flags: review?(liuche)
Assignee

Comment 84

6 years ago
Part five of the increasingly inaccurately named search preferences trilogy.

Optional extra patch to make the code in this test less incomprehensible.
Attachment #785501 - Flags: review?(liuche)
Assignee

Comment 85

6 years ago
Alas, the run went orange, but for a different reason. testAddonManager, this time. It is not clear why these failures were not observed in the earlier try run.
Time to patch testAddonManager, then...
Assignee

Comment 86

6 years ago
It appears this problem is a regression caused by botching the patch splitting. Submitted a try run with the complete patches:
https://tbpl.mozilla.org/?tree=Try&rev=2cf15eb5e16f

This ought to turn green. Let's see.
Chris, can you update the patches to reflect the patches are currently reflected in this try build, so if it greens, I know what to review? And if the current patches also match the ones applied in the try push, just let me know that.
Flags: needinfo?(ckitching)
Comment on attachment 785500 [details] [diff] [review]
Part 4/3: Correct the way in which testAddSearchEngine parses JSON to prevent test failures.

Review of attachment 785500 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits.

::: mobile/android/base/tests/testAddSearchEngine.java.in
@@ +5,5 @@
>  import android.view.View;
>  import android.widget.ListAdapter;
>  import android.widget.ListView;
>  import java.util.ArrayList;
> +import android.util.Log;

Move this up to the rest of the android.* imports.

@@ +44,5 @@
> +        try {
> +            // Parse the data to get the number of searchengines.
> +            searchEngines = getSearchEnginesNames(eventData);
> +        } catch (JSONException e) {
> +            mAsserter.ok(false, "Fatal exception in testAddSearchEngine while decoding JSON search engine string from Gecko prior to addition of new engine.", e.toString());

Can you use the Assert.fail method with mAsserter?

@@ +87,5 @@
> +        try {
> +            // Parse the data to get the number of searchengines
> +            searchEngines = getSearchEnginesNames(eventData);
> +        } catch (JSONException e) {
> +            mAsserter.ok(false, "Fatal exception in testAddSearchEngine while decoding JSON search engine string from Gecko after adding of new engine.", e.toString());

Same, use mAsserter.fail. (See javadocs for Assert.)
Attachment #785500 - Flags: review?(liuche) → review+
The try build is failing on a missing resource.
Assignee

Comment 90

6 years ago
(In reply to Chenxia Liu [:liuche] from comment #89)
> The try build is failing on a missing resource.

So I see. This one is proving less simple than expected. The resource it complains about is never referenced from anywhere in our codebase. I suspect this problem is created by the favicon size increasing patch - perhaps android is doing something deranged in secret that has it request a nonexistent resource.

But.. The resource in question is builtin and has existed since Android 1.0. Googling around suggests this error is a madness Android sometimes spits at you when you do certain flavours of slightly stupid thing. Investigation continues.(In reply to Chenxia Liu [:liuche] from comment #87)


> Chris, can you update the patches to reflect the patches are currently
> reflected in this try build, so if it greens, I know what to review? And if
> the current patches also match the ones applied in the try push, just let me
> know that.

Yes, the patches in the try push match the current ones here. Apologies for the renaming. Slightly beside the point now it went orange again.
At least it's differently broken now. That's usually progress.
Flags: needinfo?(ckitching)
Comment on attachment 785501 [details] [diff] [review]
Part 5/3: Cleanup code in testAddSearchEngine.

Review of attachment 785501 [details] [diff] [review]:
-----------------------------------------------------------------

r+, tiny nits. Fold this patch into the one before it, since this is only comments and very minor changes.

::: mobile/android/base/tests/testAddSearchEngine.java.in
@@ +33,3 @@
>          waitForText("Browser Blank Page 01");
>  
> +        // Get the searchengine data by clicking the awesomebar - this Gecko to send Java the list

Accidentally a word.

@@ +63,4 @@
>          mAsserter.dumpLog("Long Clicking at width = " + String.valueOf(width) + " and height = " + String.valueOf(height));
>          mSolo.clickLongOnScreen(width,height);
>          if (!waitForText("Add Search Engine")) {
> +            // TODO: clickLongOnScreen does not always work - known Robotium issue - . Clicking a second time seems to work.

Get rid of the extra period, if you're going to touch this line anyways.

@@ +147,5 @@
> +                            Class searchEngineRow = classLoader.loadClass("org.mozilla.gecko.SearchEngineRow");
> +                            for (int i = 0; i < adapter.getCount(); i++ ) {
> +                                View item = view.getChildAt(i);
> +                                if (searchEngineRow.isInstance(item)) {
> +                                    searchEngineCount++;

All these lines look identical on visual inspection. Has something changed...?
Attachment #785501 - Flags: review?(liuche) → review+
Assignee

Comment 93

6 years ago
No problem - I'll take care of the nits in due course. Always good to get cleanup patches landed!

(In reply to Chenxia Liu [:liuche] from comment #91)
> 
> @@ +147,5 @@
> > +                            Class searchEngineRow = classLoader.loadClass("org.mozilla.gecko.SearchEngineRow");
> > +                            for (int i = 0; i < adapter.getCount(); i++ ) {
> > +                                View item = view.getChildAt(i);
> > +                                if (searchEngineRow.isInstance(item)) {
> > +                                    searchEngineCount++;
> 
> All these lines look identical on visual inspection. Has something
> changed...?

These lines were indented one level further than they should have been.
https://hg.mozilla.org/mozilla-central/rev/7bbaba1b712f
https://hg.mozilla.org/mozilla-central/rev/b92db71dc525
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Firefox 25 → Firefox 26
Flags: in-moztrap?(fennec)
Blocks: 902258
Reporter

Updated

6 years ago
Depends on: 902932
Reporter

Updated

6 years ago
Depends on: 904279
Reporter

Updated

6 years ago
No longer depends on: 904279
Status: RESOLVED → VERIFIED
Reporter

Updated

6 years ago
Depends on: 910189

Comment 95

6 years ago
Test case created in moztrap:
https://moztrap.mozilla.org/manage/cases/?filter-suite=407
Flags: in-moztrap?(fennec) → in-moztrap+
You need to log in before you can comment on or make changes to this bug.