343707 - Add Restore Default Search Engines button to search manager

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Michael Wu [:mwu]]

We currently have a checkbox to restore default search engines in the safemode dialog. We also need a button which does the same thing in the search manager dialog.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

The tricky part here is that once bug 335102 lands, the hidden engines may have bogus placeholder names when they are restored, since a user may have renamed a non-hidden engine to the name of a hidden engine. We need to figure out how to deal with that.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

There's no UI design needed here, right? It's just a matter of figuring out how to make it work?

Comment 3

[Radek 'sysKin' Czyz]

How about just adding a category to a.m.o/search-engines.php?

The list should of course depend on locale (which, I notice, is not sent as GET data - why not, sounds useful).

Comment 4

[Joey Minta]

The implementation of this is going to depend on bug 335102 landing, so adding dependency.

Comment 5

[Mike Connor [:mconnor]]

-> mwu (this might hurt, just a bit)

Comment 6

[Michael Wu [:mwu]]

Attachment: Add restore default search engines functionality to engine manager

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 230526 [details] [diff] [review]
Add restore default search engines functionality to engine manager

>Index: browser/components/search/content/engineManager.js

>+  onRestoreDefaults: function engineManager_onRestoreDefaults() {
>+    var num = gEngineView._engineStore.restoreDefaultEngines();
>+    gEngineView.rowCountChanged(gEngineView.lastIndex, num);

Shouldn't this be rowCountChanged(0, num)? restoreDefaultEngines inserts items starting at index 0.

>+  restoreDefaultEngines: function ES_restoreDefaultEngines() {
>+    var searchService = Cc["@mozilla.org/browser/search-service;1"].
>+                        getService(Ci.nsIBrowserSearchService);
>+    var defaultEngines = searchService.getDefaultEngines({});
>+    var i = 0;
>+    var e;
>+    function checkHidden (element) {
>+      return element.originalEngine == e;
>+    }
>+    for each (e in defaultEngines) {
>+      if (this._engines.some(checkHidden))
>+        continue;

Naming the function checkHidden is kind of confusing, it sounds related to the "hidden" attribute. Perhaps isEqual? isSameEngine? isCloneOfCurrentEngine? And I'd use "engineClone" instead of "element" as the param name.

I think I prefer using extra2 instead of extra1 for the "Restore Defaults" button. We might also want it to be "Restore default engines", but beltzner/mconnor can let you know what it should be.

There are two other issues that I think would be good to fix:
1) Only enable the button when one of the defaults is hidden (should be easy to do now that you have getDefaultEngines), because otherwise it does nothing and could be confusing.
2) Restore the engines in their original order, instead of in load order (alphabetically). This might be a bit trickier... you can probably duplicate some of the pref-getting code from _convertOldPrefs in nsSearchService.js.

Comment 8

[Michael Wu [:mwu]]

(In reply to comment #7)
> (From update of attachment 230526 [details] [diff] [review] [edit])
> >Index: browser/components/search/content/engineManager.js
> 
> >+  onRestoreDefaults: function engineManager_onRestoreDefaults() {
> >+    var num = gEngineView._engineStore.restoreDefaultEngines();
> >+    gEngineView.rowCountChanged(gEngineView.lastIndex, num);
> 
> Shouldn't this be rowCountChanged(0, num)? restoreDefaultEngines inserts items
> starting at index 0.
> 
Assertions say otherwise. (don't know why)

> >+  restoreDefaultEngines: function ES_restoreDefaultEngines() {
> >+    var searchService = Cc["@mozilla.org/browser/search-service;1"].
> >+                        getService(Ci.nsIBrowserSearchService);
> >+    var defaultEngines = searchService.getDefaultEngines({});
> >+    var i = 0;
> >+    var e;
> >+    function checkHidden (element) {
> >+      return element.originalEngine == e;
> >+    }
> >+    for each (e in defaultEngines) {
> >+      if (this._engines.some(checkHidden))
> >+        continue;
> 
> Naming the function checkHidden is kind of confusing, it sounds related to the
> "hidden" attribute. Perhaps isEqual? isSameEngine? isCloneOfCurrentEngine? And
> I'd use "engineClone" instead of "element" as the param name.
> 
Sure.

> I think I prefer using extra2 instead of extra1 for the "Restore Defaults"
> button. We might also want it to be "Restore default engines", but
> beltzner/mconnor can let you know what it should be.
> 
Beltzner decided on "Restore Defaults" in the other bug. I'll switch to extra2.

> There are two other issues that I think would be good to fix:
> 1) Only enable the button when one of the defaults is hidden (should be easy to
> do now that you have getDefaultEngines), because otherwise it does nothing and
> could be confusing.
Can do.

> 2) Restore the engines in their original order, instead of in load order
> (alphabetically). This might be a bit trickier... you can probably duplicate
> some of the pref-getting code from _convertOldPrefs in nsSearchService.js.
> 
The code restores engines in whatever order the user had them in before, except at the top of the list. That's the difference between this restore defaults button and the one in safe mode. Inserting them in the right place in the list is tricky when there are pending move/delete ops, but the safemode dialog can put them in the right place. Neither restore functions try to restore the original order. I think putting that in should be in a new bug since the safemode dialog needs it too.

Comment 9

[Michael Wu [:mwu]]

Attachment: Add restore default search engines functionality to engine manager, v2

Updated w/ Gavin's comments.

Comment 10

[Michael Wu [:mwu]]

Attachment: Add restore default search engines functionality to engine manager, v3

This version does proper ordering on restore.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 231457 [details] [diff] [review]
Add restore default search engines functionality to engine manager, v3

>Index: browser/components/search/nsIBrowserSearchService.idl

>   /**
>    * Un-hides all engines installed in the directory corresponding to
>-   * the directory service's NS_APP_SEARCH_DIR key.
>+   * the directory service's NS_APP_SEARCH_DIR key. For more control,
>+   * use getDefaultEngines.

I think something like "(i.e. the set of engines returned by getDefaultEngines)" would be a better comment.

>Index: browser/components/search/nsSearchService.js

> const BROWSER_SEARCH_PREF = "browser.search.";
>+const BROWSER_SEARCH_ORDER_PREF = "browser.search.order.";

No need for this, as mentioned before.

>+  getDefaultEngines: function SRCH_SVC_getDefault(aCount) {
>+    var prefB = Cc["@mozilla.org/preferences-service;1"].
>+                getService(Ci.nsIPrefBranch);

Please use the default pref branch, and only get the search branch, like:
getService(nsIPrefService).getDefaultBranch(BROWSER_SEARCH_PREF).

>+    var engines = this._sortedEngines.filter(function (engine) {
>+                                        return engine._isInAppDir;
>+                                      });

I think putting the function definition on it's own (single) line, and name it something like isDefault is easier to read. My fault for not doing this elsewhere in the file.

>+    var engineOrder = new Array();

No need for an array, you're just using generic object properties. I also tend to favor empty object literals instead of the explicit constructor, so just do:
var engineOrder = {};

>+    var i = 1;
>+    while (true) {
>+      try {
>+        var engineName = prefB.getCharPref(BROWSER_SEARCH_ORDER_PREF + i);

nit: declare engineName before the loop? This can also just be getCharPref("order." + i) now that you're using the right branch.

>+    engines.sort(function (a, b) {
>+              var aIdx = engineOrder[a.name];
>+              var bIdx = engineOrder[b.name];
>+              if (aIdx > 0 && bIdx > 0) {
>+                return aIdx - bIdx;
>+              } else if (aIdx > 0) {
>+                return 1;
>+              } else if (bIdx > 0) {
>+                return -1;
>+              } else {
>+                if (a.name < b.name)
>+                  return -1;
>+                else
>+                  return 1;
>+              }
>+            });

Again, declare the function beforehand and give it a name. I think a |switch (true)| instead of an |if/else if| tree makes this a bit clearer, but up to you I guess. Get rid of the else-after-return.

>Index: browser/components/search/content/engineManager.js

> function EngineStore() {
>   var searchService = Cc["@mozilla.org/browser/search-service;1"].
>                       getService(Ci.nsIBrowserSearchService);
>   this._engines = searchService.getVisibleEngines({}).map(this._cloneEngine);
>+  this._defaultEngines = searchService.getDefaultEngines({}).map(this._cloneEngine);
> 
>   this._ops = [];
>+
>+  // check if we need to disable the restore defaults button
>+  var allVisible = true;
>+  for each (var e in this._defaultEngines) {
>+    if (!this._engines.some(this._isSameEngine, e)) {
>+      allVisible = false;
>+      break;
>+    }
>+  }
>+  if (allVisible)
>+    this.defaultEnginesVisible = true;

Can't you just check to see if one of the engines in _defaultEngines is hidden, instead of comparing both arrays?

>+  set defaultEnginesVisible(val) {
>+    document.documentElement.getButton("extra2").setAttribute("disabled", val);
>+    return val;
>   },

Code related to the dialog itself doesn't really belong on the EngineStore object. Can you put this function on gEngineManagerDialog and call it there?

>+  _isSameEngine: function ES_isSameEngine(aEngineClone) {
>+      return aEngineClone.originalEngine == this.originalEngine;
>+  },

Using "this" here is somewhat of a hack, and is certainly confusing. I'd declare this function in the global scope, and add a comment that callers need to pass the correct thisObj.

I think the rest looks OK, but I'd like to take another look at an updated patch.

Comment 12

[Michael Wu [:mwu]]

(In reply to comment #11)
> >+  _isSameEngine: function ES_isSameEngine(aEngineClone) {
> >+      return aEngineClone.originalEngine == this.originalEngine;
> >+  },
> 
> Using "this" here is somewhat of a hack, and is certainly confusing. I'd
> declare this function in the global scope, and add a comment that callers need
> to pass the correct thisObj.
> 
It doesn't feel quite right in global scope. I added a comment about the use of "this".

I've applied all other suggestions. Thanks for the review!

Comment 13

[Michael Wu [:mwu]]

Attachment: Add restore default search engines functionality to engine manager, v4

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 232011 [details] [diff] [review]
Add restore default search engines functionality to engine manager, v4

>Index: browser/components/search/nsIBrowserSearchService.idl

>+   * the directory service's NS_APP_SEARCH_DIR key. (e.g. the set of
>+   * engines returned by getDefaultEngines)

i.e., not e.g.

>Index: browser/components/search/nsSearchService.js

>+  getDefaultEngines: function SRCH_SVC_getDefault(aCount) {

>+    function compareEngines (a, b) {
>+      var aIdx = engineOrder[a.name];
>+      var bIdx = engineOrder[b.name];
>+      if (aIdx && bIdx)
>+        return aIdx - bIdx;
>+      else if (aIdx)
>+        return -1;
>+      else if (bIdx)
>+        return 1;
>+      else
>+        return a.name < b.name ? -1 : 1;
>+    }

get rid of all the elses :)

>     aCount.value = engines.length;
>     return engines;

I think you can cache this result, since we can guarantee that this list won't ever change in a given session. Probably doesn't matter, though, since this likely won't get called a lot.

>Index: browser/components/search/content/engineManager.js

>+  showRestoreDefaults: function engineManager_showRestoreDefaults(val) {
>+    document.documentElement.getButton("extra2").setAttribute("disabled", !val);

Use the property instead of the attribute.

> function EngineStore() {
>   var searchService = Cc["@mozilla.org/browser/search-service;1"].
>                       getService(Ci.nsIBrowserSearchService);
>   this._engines = searchService.getVisibleEngines({}).map(this._cloneEngine);
>+  this._defaultEngines = searchService.getDefaultEngines({}).map(this._cloneEngine);
> 
>   this._ops = [];
>+
>+  // check if we need to disable the restore defaults button
>+  var allVisible = true;
>+  for each (var e in this._defaultEngines) {
>+    if (e.hidden) {
>+      allVisible = false;
>+      break;
>+    }
>+  }
>+  gEngineManagerDialog.showRestoreDefaults(!allVisible);
> }

var someHidden = this._defaultEngines.some(function (e) {return e.hidden;});
gEngineManagerDialog.showRestoreDefaults(someHidden);

>+  restoreDefaultEngines: function ES_restoreDefaultEngines() {
>+    var i = 0;
>+    for each (var e in this._defaultEngines) {
>+      // skip adding engine if the engine is already in the list
>+      if (this._engines.some(this._isSameEngine, e))
>+        continue;
>+
>+      this._engines.splice(i, 0, e);
>+      this._ops.push(new EngineUnhideOp(e, i));
>+      i++;
>+    }
>+    gEngineManagerDialog.showRestoreDefaults(false);
>+    return i;
>   },

Should this move existing app engines to their original position? I think you can just call this.moveEngine(engine, i); before the continue to have that happen.

Comment 15

[Michael Wu [:mwu]]

Attachment: Add restore default search engines functionality to engine manager, v5

(In reply to comment #14)
> (From update of attachment 232011 [details] [diff] [review] [edit])
> >     aCount.value = engines.length;
> >     return engines;
> 
> I think you can cache this result, since we can guarantee that this list won't
> ever change in a given session. Probably doesn't matter, though, since this
> likely won't get called a lot.
> 
Rather not have to complicate things.

> >+  restoreDefaultEngines: function ES_restoreDefaultEngines() {
> >+    var i = 0;
> >+    for each (var e in this._defaultEngines) {
> >+      // skip adding engine if the engine is already in the list
> >+      if (this._engines.some(this._isSameEngine, e))
> >+        continue;
> >+
> >+      this._engines.splice(i, 0, e);
> >+      this._ops.push(new EngineUnhideOp(e, i));
> >+      i++;
> >+    }
> >+    gEngineManagerDialog.showRestoreDefaults(false);
> >+    return i;
> >   },
> 
> Should this move existing app engines to their original position? I think you
> can just call this.moveEngine(engine, i); before the continue to have that
> happen.
> 
It doesn't seem to be that trivial. The safemode engine restore also needs to do a proper reorder, so I would prefer to fix up the reordering issues in a follow up bug.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 232379 [details] [diff] [review]
Add restore default search engines functionality to engine manager, v5

>+  // Callback for Array's some(). A thisObj must be passed to some()
>+  _isSameEngine: function ES_isSameEngine(aEngineClone) {
>+      return aEngineClone.originalEngine == this.originalEngine;
>+  },

nit: bad indent here.

(In reply to comment #8)
> > Shouldn't this be rowCountChanged(0, num)? restoreDefaultEngines inserts
> > items starting at index 0.
>
> Assertions say otherwise. (don't know why)

Which assertions? I tried this in my own build and didn't get any assertions. It also fixed a bug where the selection isn't properly maintained after restoring defaults.

One other minor issue: if the current engine is a default, and you remove and then restore it in the dialog, when you close the dialog the current engine is changed, since it was removed during the commit and then restored. This can be fixed by saving the current engine before committing the ops, and then restoring it afterwards if it's not hidden.

r=me with those issues fixed/explained as needed.

Comment 17

[Michael Wu [:mwu]]

(In reply to comment #16)
> (In reply to comment #8)
> > > Shouldn't this be rowCountChanged(0, num)? restoreDefaultEngines inserts
> > > items starting at index 0.
> >
> > Assertions say otherwise. (don't know why)
> 
> Which assertions? I tried this in my own build and didn't get any assertions.
> It also fixed a bug where the selection isn't properly maintained after
> restoring defaults.
> 
Odd, I don't get the assertions now.

Comment 18

[Michael Wu [:mwu]]

Attachment: Add restore default search engines functionality to engine manager, v6

Comment 19

[Michael Wu [:mwu]]

Attachment: Add restore default search engines functionality to engine manager, v7

Crap, forgot the locale changes.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/search/content/engineManager.xul 	1.7
mozilla/browser/components/search/content/engineManager.js 	1.11
mozilla/browser/components/search/nsSearchService.js 	1.72
mozilla/browser/components/search/nsIBrowserSearchService.idl 	1.14
mozilla/browser/locales/en-US/chrome/browser/engineManager.dtd 	1.4

Comment 21

[Axel Hecht [:Pike]]

This rev's the interface ID, is that a good thing to do on the branch? Or do we need a branch patch for this?

Comment 22

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #21)
> This rev's the interface ID, is that a good thing to do on the branch? Or do we
> need a branch patch for this?

No, this interface has never been shipped and thus can be changed without problem.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 232624 [details] [diff] [review]
Add restore default search engines functionality to engine manager, v7

a=drivers, please land on the branch.

This isn't actually the ideal solution, since as I understand the patch, it is going to dismiss the dialog when a user clicks "restore defaults", which will not be what a user expects to have happen.

I'm going to file a followup bug about this.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 232624 [details] [diff] [review]
Add restore default search engines functionality to engine manager, v7

mwu tells me that it doesn't actually dismiss the dialog, so this is now pretty OK!

Comment 25

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/search/content/engineManager.xul 	1.1.2.10
mozilla/browser/components/search/content/engineManager.js 	1.1.2.13
mozilla/browser/components/search/nsSearchService.js 	1.1.2.60
mozilla/browser/components/search/nsIBrowserSearchService.idl 	1.1.2.15
mozilla/browser/locales/en-US/chrome/browser/engineManager.dtd 	1.1.2.5