335102 - make it possible to rename search engine (support search plugin renaming)

Status: RESOLVED WONTFIX

(Firefox :: Search, enhancement, P5)

Description

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

This requires using something other than the display name as the engine's unique ID.

Comment 1

[Joe Hughes]

Perhaps make the search engine ID the URI of the engine definition file?

Comment 2

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

I think that's a good idea. I likely won't have time to do this work, but I'd be glad to lend a hand to anyone who wants to take this.

Comment 3

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

Keying the search plugins by URI would also solve the problem of not being able to install two search engines with the same name (e.g you can't install another Google engine after "removing"/hiding the one shipped by default).

Comment 4

[Régis Caspar]

Joey, I made a patch allowing search engine renaming with the current indexation method (i.e. names) I then saw the "ID patch" in bug 340331. So if needed, I can provide a base patch you can modify or I even can modify it once 340331 lands.

Comment 5

[Joey Minta]

Attachment: rename engines

Patch implements search engine renaming.  We still block users from installing engines with duplicate names (unless the engine is hidden).  It also turned out that I had to go ahead and fix the "remove engine metadata when removing an engine" bug in order to prevent a regression there.  Note that I still had to switch to using _id internally, since names are now mutable and modifying the engineStore isn't much fun.

Comment 6

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

Comment on attachment 226444 [details] [diff] [review]
rename engines

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

>   /**
>+   * Renames the engine to a new name.  Note that this will throw in the case
>+   * where the new name is already being used.

I'd just say "Renames the specified engine.", with an @throws to describe the case where it fails.

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

>   get name() {
>+    try { 
>+      // We don't always have an _id early in load
>+      var overrideName = engineMetadataService.getAttr(this, "engineName");
>+      if (overrideName)
>+        this._name = overrideName;
>+    } catch(ex) {}
>     return this._name;
>   },

Discussed this on IRC, I think we should keep _originalName (or something) seperately, to hold the engine name that's in the description file. This getter can then return that if we don't yet have an ID.

>-    if (aEngine.name in this._engines) {
>+    if (this.getEngineByName(aEngine.name)) {

(aEngine._id in this._engines) ?

>   getEngineByName: function SRCH_SVC_getEngineByName(aEngineName) {
>-    return this._engines[aEngineName] || null;
>+    for each (engine in this._engines) {

Forgot a "var" here before engine.

>+    engineMetadataService.deleteEngineData(engineToRemove, "order");
>+    engineMetadataService.deleteEngineData(engineToRemove, "alias");
>+    // Don't delete engineName, because it might create a duplicate

Discussed this on IRC too, we just want to delete order here, not alias, but we should delete all attributes in the else block below.

>     if (engineToRemove._readOnly) {
>       // Just hide it (the "hidden" setter will notify)
>       engineToRemove.hidden = true;
>     } else {

>+    var dupeEngine = this.getEngineByName(aName);
>+    if (dupeEngine) {
>+      if (engineMetadataService.getAttr(dupeEngine, "hidden")) {
>+        // Rename the hidden engine so that we can rename this visible one
>+        var uniqueID = 1;
>+        while (this.getEngineByName(aName + uniqueID.toString()))
>+          uniqueID++;

So, this is kinda problematic for the case where we want to restore a hidden engine. Maybe we should just set the name to null, or something, and keep the original name around as an attribute? Then when we restore it we can restore it as "OriginalName (Restored)" or something like that?

>+        var newName = aName + uniqueID.toString();
>+        engineMetadataService.setAttr(dupeEngine, "engineName", newName);
>+      } else
>+        throw Cr.NS_ERROR_FAILURE;

add a LOG() here?

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

>+  renameCallback: function engineManager_rename(aNewName) {
>+    gEngineView._engineStore.renameEngine(gEngineView.selectedEngine, aNewName);
>+    document.getElementById("engineList").boxObject
>+                                         .invalidateRow(gEngineView.selectedIndex);

Should create an invalidateRow helper on EngineView, much like the current invalidate().

>   onSelect: function engineManager_onSelect() {
>     // buttons only work if an engine is selected and it's not the last engine
>     var disableButtons = (gEngineView.selectedIndex == -1) ||
>                          (gEngineView.lastIndex == 0);
>+    var engineSelected = (gEngineView.selectedIndex == -1)

This should be "noEngineSelected". Also you can put this before disableButtons, and use it there. And don't forget the semi-colon :)

>+EngineRenameOp.prototype = {
>+  _engine: null,
>+  _newName: null,
>+  commit: function ERNO_commit() {
>+    var searchService = Cc["@mozilla.org/browser/search-service;1"].
>+                        getService(Ci.nsIBrowserSearchService);
>+    searchService.renameEngine(this._engine, this._newName);
>+  }
>+}

Should there be safeguards here in case renameEngine fails? Or are we saying that that won't ever happen because engineRename.xul doesn't allow it? If so, add a comment that we rely on not being passed names that will make renameEngine throw.

>Index: browser/components/search/content/renameEngine.xul

>+<script type="application/x-javascript"><![CDATA[
>+    var gEngineNames;
>+    function renameEngineLoad() {
>+      document.getElementById("engineName").value = window.arguments[0];
>+      gEngineNames = window.arguments[1].filter(function(a) { return (a != window.arguments[0]); });

Should this filter be done by the caller?

>+    function onInput(aValue) {
>+      var disableOK = !aValue || (gEngineNames.indexOf(aValue.toLowerCase()) != -1)
>+      var okButton = document.getElementById("renameEngine").getButton("accept");

document.documentElement?

Comment 7

[Joey Minta]

Attachment: rename engines v2

(This time on the right bug.) Patch addresses previous review comments as well as fixing some js-assertions during search engine installation by switching to ._originalName for logging/file name creation.

Comment 8

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

Comment on attachment 226662 [details] [diff] [review]
rename engines v2

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

Can you declare _originalName explicitly at the top of the Engine object, with a comment?

>   get name() {
>-    return this._name;
>+    try { 
>+      // We don't always have an _id early in load
>+      var overrideName = engineMetadataService.getAttr(this, "engineName");
>+      if (overrideName)
>+        this._name = overrideName;
>+    } catch(ex) {}
>+    return this._name || this._originalName;

This gets the attribute each time. Shouldn't this instead be something like:
if (!this._name) {
  try {
    this._name = (get from db)
  } catch {
    return this._originalName;
  }
}
return this._name;

>-    if (aEngine.name in this._engines) {
>+    if (this.getEngineByName(aEngine.name)) {

(aEngine._id in this._engines) ?

>     if (engineToRemove == this.currentEngine)
>       this._currentEngine = null;
> 
>+    engineMetadataService.deleteEngineData(engineToRemove, "order");

Comment that we're removing the order attribute regardless of whether we're hiding or removing? And actually, it would be clearer to move this into the if (_readOnly) block below, if you add the "remove all engine metadata" method mentioned below.

>+      engineMetadataService.deleteEngineData(engineToRemove, "hidden");
>+      engineMetadataService.deleteEngineData(engineToRemove, "alias");
>+      engineMetadataService.deleteEngineData(engineToRemove, "engineName");

Seems like what we really want here is a "clear all engine data" function. It's probably also a good idea to add constants for the supported attribute names (hidden, alias, engineName, etc) and declare them all in one place so it's easy to see which attributes we store.

>+  renameEngine: function SRCH_SVC_renameEngine(aEngine, aName) {
>+    ENSURE_ARG(aEngine, "no engine passed to renameEngine!");
>+    ENSURE_ARG(aName, "no name passed to renameEngine!");
>+
>+    var dupeEngine = this.getEngineByName(aName);
>+    if (dupeEngine) {
>+      if (engineMetadataService.getAttr(dupeEngine, "hidden")) {

Does getAttr return really return a boolean? I would think it returns "false" or "true", both of which will pass this test.

>+        // Rename the hidden engine so that we can rename this visible one
>+        // Use the ._orginalName property when restoring the engine, but make
>+        // sure to check for duplicate names!
>+        var uniqueID = 1;
>+        while (this.getEngineByName(aName + uniqueID.toString()))
>+          uniqueID++;
>+        var newName = aName + uniqueID.toString();

Would it be a good idea to change it's name to something like moz-engine-renamed-<number>, instead of just appending to the old name? That name is being discarded anyways, right? Keeping it around might be confusing, and having some explicitly bogus value will be more informative to anyone who forgets to use _originalName when restoring it.

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

>   onSelect: function engineManager_onSelect() {
>     // buttons only work if an engine is selected and it's not the last engine
>+    var noEngineSelected = (gEngineView.selectedIndex == -1);
>     var disableButtons = (gEngineView.selectedIndex == -1) ||

You can use noEngineSelected in the disableButtons declaration.

>+  invalidateRow: function engineView_invalidateRow(aRow) {
>+    this.tree.invalidateRow(gEngineView.selectedIndex);
>+  },

Either use aRow, or remove aRow and name this invalidateSelectedRow() or something (and fix the caller).

r- because of the bad dupe check in renameEngine and the name getter, but the rest all looks good.

Comment 9

[Joey Minta]

(In reply to comment #8)

> 
> >-    if (aEngine.name in this._engines) {
> >+    if (this.getEngineByName(aEngine.name)) {
> 
> (aEngine._id in this._engines) ?
Sorry, I thought I included a comment about this before.  What we really want is to ensure that the name is unique, not that the engine isn't installed already, so I still think getEngineByName is the right answer (assuming this was grabbed from _addEngineToStore).

Comment 10

[Joey Minta]

Attachment: rename engines v3

Patch updated to reflect previous review comments.  mozStorage is smart enough to handle true/false values (and actually returns them as 1/0) so that part works fine.  Also fixed an edge case where people rename an engine to its own name.  This shouldn't throw.

Comment 11

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

Comment on attachment 226856 [details] [diff] [review]
rename engines v3

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

>+// The attributes (metadata) we keep about engines
>+const kEngineAttrs = ["alias", "order", "hidden", "engineName"];

ENGINE_ATTRS

>-      for (var i = 0; i < lines.length; i++) {
>+      for (var i = 0; i < lines.length; i++) {_id

Oops?

>-    return this._name;
>+    if (!this._name) {
>+      try { 
>+        // We don't always have an _id early in load
>+        var overrideName = engineMetadataService.getAttr(this, "engineName");
>+        if (overrideName)
>+          this._name = overrideName;
>+      } catch(ex) {}
>+    }
>+    return this._name || this._originalName;

I think it'd be clearer that _originalName is only returned when we can't get the attribute from the DB if |return this._originalName| was in the catch block instead.

>+  renameEngine: function SRCH_SVC_renameEngine(aEngine, aName) {

>+    var engine = aEngine.wrappedJSObject;
>+    engineMetadataService.setAttr(engine, "engineName", aName);

Need to set engine._name here, too, otherwise the name won't update in the UI since it's cached on the engine object.

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

>+  renameCommand: function engineManager_renameCom() {
>+    var engineName = gEngineView.selectedEngine.name;
>+    var allNames = [];
>+    for (var i in gEngineView._engineStore.engines) {
>+        if (!gEngineView._engineStore.engines[i])
>+          continue;

When can engines[i] be null? I think this should just be a normal |for (;;)| loop. Fix the indent too (2 spaces).

You need to add a splice() for tempNames in ES_removeEngine so that the engine name is removed when the engine is, otherwise things get weird when you rename and then remove an engine. Actually, since the objects held in the engine store's _engines array are "clone" objects and not the actual nsISearchEngine objects, can't you just use the name property on those directly, and get rid of tempNames entirely?

The rename dialog looks kinda weird on Windows: it's pretty small, and the "Name" label is not vertically centered with the text box. That can be tweaked separately of course.

Comment 12

[Joey Minta]

Attachment: rename engines v4

Patch updated to reflect previous review comments.  I've removed tempNames from the engineManager.js code and also tweaked renameEngine.xul to at least have the 'Name' label aligned properly.

Comment 13

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

Comment on attachment 227111 [details] [diff] [review]
rename engines v4

>+    // Filter out any nulls for engines that may have been removed
>+    this._sortedEngines = this._sortedEngines.filter(function(a) { return a; });

Can you put this before the alphaEngines stuff? It'd be clearer that it's filtering out any holes left by sorted engines being gone (and the alphabetical sorting stuff only takes effect on the first start, anyways).

Comment 14

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

Attachment: rename engines v4.1

This is the same patch, unbittrotted, and including the locale changes.

Comment 15

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

This patch doesn't seem to work. I can do a ui-review with a set of screenshots if someone can supply those ...

Comment 16

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

Comment on attachment 230084 [details] [diff] [review]
rename engines v4.1

Yeah, this is missing a lot of stuff.

Comment 17

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

I don't think it's a good idea to land this patch at this point in the release Firefox 2 release cycle, for the following reasons:

1) It changes the search service code in some major ways, which indicates to me that it carries a fairly large risk of regression.
2) The original patch has been bitrotted by many of the other search service changes, and updating it is not trivial because of some of the conflicts.
3) It makes implementing bug 343707 (a [mustfix] bug) much harder.
4) This patch would benefit from a lot of baking time on the trunk, which it won't be able to get if the target is really Firefox 2 Beta 2.

Given these, I'm requesting reconsideration of the blocking-firefox2 status. I don't think that this feature is compelling enough to be worth the risk at this point in the release process.

Comment 18

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

Convincing arguments, Gavin!

Comment 19

[Jo Hermans]

*** Bug 356726 has been marked as a duplicate of this bug. ***

Comment 20

[Joey Minta]

Releasing back to nobody@ to keep shaver happy :-)

Comment 21

[Andrés Delfino]

Nominating for blocking‑firefox3.1 now that we have some time.

Comment 22

[Mike Connor [:mconnor]]

Not blocking, probably not a bad idea, but not a big deal anyway.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Gavin, is qawanted still needed?

Comment 24

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

qawanted was added by me, asking to change target milestone. I think I wronged uncorrect, since only devs must change milestone, right?

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Nothing which QA should care about. Lets remove the keyword to get it from my list. Thanks Lucas.

Comment 26

[philip_hoflack]

It seems that this idea has been forgotten, but it's a small and handy feature. That's why I'm bumping it.

Comment 27

[Mark Banner (:standard8)]

We are not intending on supporting this at the current time. There doesn't seem to be a good reason for the user to need to change it, if it is an engine they don't like they can uninstall it. Also, there doesn't seem to have been any demand for it.

Comment 28

[zim78]

I don't agree. Customizing search-engine names doesn't cost much to implement and allows for more consistent and concise naming (e.g., YouTube instead of 'YouTube Video Search').

Comment 29

[Christian]

Not sure if anything's changed on the complexity of this request, but there's one scenario that I don't believe has been covered.

Specifically, users who are creating a search engine from sites that use subdomain-specific search URLs can't create a search engine for each subdomain. This is especially problematic with sites like JIRA which, when hosted, use a subdomain for each account.

E.g., https://accountone.atlassian.net/searchurl; https://accounttwo.atlassian.net/searchurl are both JIRA URLs and have the same name: JIRA. The problem with that is that you can't add a search engine for "accounttwo" if you've already added one for "accountone" because they have the same name.

One "workaround" would be to use a UUID for the engines (or URL) rather than Name to differentiate the engines. Then a user can use keywords to specify which engine they want to use.

I'm not sure how many use this feature—I just discovered the possibility of adding a custom JIRA search after years of working with JIRA using FF.

Christian

Comment 30

[Mark Banner (:standard8)]

(In reply to Christian from comment #29)

E.g., https://accountone.atlassian.net/searchurl; https://accounttwo.atlassian.net/searchurl are both JIRA URLs and have the same name: JIRA. The problem with that is that you can't add a search engine for "accounttwo" if you've already added one for "accountone" because they have the same name.

I see this mainly as a Jira issue. They should be generating different names here, not only as it would avoid this problem, but that it would give better context to the user, e.g. "Jira - accountone" is much better context than just "Jira".