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

NEW
Unassigned

Status

()

enhancement
P5
normal
Rank:
59
13 years ago
3 years ago

People

(Reporter: Gavin, Unassigned)

Tracking

(Blocks 3 bugs, {helpwanted})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 -
blocking-firefox2 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fxsearch])

Attachments

(5 obsolete attachments)

This requires using something other than the display name as the engine's unique ID.
Severity: normal → enhancement
Status: NEW → ASSIGNED

Comment 1

13 years ago
Perhaps make the search engine ID the URI of the engine definition file?
Blocks: 335435
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.
Assignee: gavin.sharp → nobody
Status: ASSIGNED → NEW
Keywords: helpwanted
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).
Flags: blocking-firefox2?

Updated

13 years ago
Blocks: 340604
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: --- → Firefox 2 beta1
Assignee: nobody → jminta

Updated

13 years ago
Whiteboard: [swag:1d]

Updated

13 years ago
Blocks: 340122

Updated

13 years ago
No longer blocks: 340331
Depends on: 340331

Updated

13 years ago
No longer blocks: 340604

Comment 4

13 years ago
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

13 years ago
Posted patch rename engines (obsolete) — Splinter Review
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.
Attachment #226444 - Flags: superreview?(mconnor)
Attachment #226444 - Flags: review?(gavin.sharp)
Attachment #226444 - Flags: approval1.8.1?
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?
Attachment #226444 - Flags: superreview?(mconnor)
Attachment #226444 - Flags: review?(gavin.sharp)
Attachment #226444 - Flags: review-
Attachment #226444 - Flags: approval1.8.1?

Comment 7

13 years ago
Posted patch rename engines v2 (obsolete) — Splinter Review
(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.
Attachment #226444 - Attachment is obsolete: true
Attachment #226662 - Flags: ui-review?(mconnor)
Attachment #226662 - Flags: review?(gavin.sharp)
Attachment #226662 - Flags: approval1.8.1?

Updated

13 years ago
Attachment #226662 - Flags: approval1.8.1?
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.
Attachment #226662 - Flags: review?(gavin.sharp) → review-

Comment 9

13 years ago
(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

13 years ago
Posted patch rename engines v3 (obsolete) — Splinter Review
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.
Attachment #226662 - Attachment is obsolete: true
Attachment #226856 - Flags: ui-review?(mconnor)
Attachment #226856 - Flags: review?(gavin.sharp)
Attachment #226662 - Flags: ui-review?(mconnor)
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.
Attachment #226856 - Flags: review?(gavin.sharp) → review-

Comment 12

13 years ago
Posted patch rename engines v4 (obsolete) — Splinter Review
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.
Attachment #226856 - Attachment is obsolete: true
Attachment #227111 - Flags: ui-review?(mconnor)
Attachment #227111 - Flags: review?(gavin.sharp)
Attachment #226856 - Flags: ui-review?(mconnor)
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).
Attachment #227111 - Flags: review?(gavin.sharp) → review+
Target Milestone: Firefox 2 beta1 → Firefox 2 beta2

Updated

13 years ago
Attachment #227111 - Flags: ui-review?(mconnor) → ui-review?(beltzner)
Whiteboard: [swag:1d] → [has patch][needs ui-review beltzner]

Updated

13 years ago
Blocks: 343707
Posted patch rename engines v4.1 (obsolete) — Splinter Review
This is the same patch, unbittrotted, and including the locale changes.
Attachment #227111 - Attachment is obsolete: true
Attachment #227111 - Flags: ui-review?(beltzner)
Attachment #230084 - Flags: ui-review?(beltzner)
This patch doesn't seem to work. I can do a ui-review with a set of screenshots if someone can supply those ...
Comment on attachment 230084 [details] [diff] [review]
rename engines v4.1

Yeah, this is missing a lot of stuff.
Attachment #230084 - Attachment is obsolete: true
Attachment #230084 - Flags: ui-review?(beltzner)
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.
Flags: blocking-firefox2+ → blocking-firefox2?
Convincing arguments, Gavin!
Flags: blocking-firefox2? → blocking-firefox2-
Whiteboard: [has patch][needs ui-review beltzner] → [has patch]

Comment 19

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

Comment 20

12 years ago
Releasing back to nobody@ to keep shaver happy :-)
Assignee: jminta → nobody
Summary: support search engine renaming → make it possible to rename search engine (support search plugin renaming)

Comment 21

11 years ago
Nominating for blocking‑firefox3.1 now that we have some time.
Flags: blocking-firefox3.1?
Keywords: qawanted
Whiteboard: [has patch] → [has patch] [change old target milestone]
Version: 2.0 Branch → Trunk
Not blocking, probably not a bad idea, but not a big deal anyway.
Flags: blocking-firefox3.1? → blocking-firefox3.1-
Gavin, is qawanted still needed?
qawanted was added by me, asking to change target milestone. I think I wronged uncorrect, since only devs must change milestone, right?
Nothing which QA should care about. Lets remove the keyword to get it from my list. Thanks Lucas.
Keywords: qawanted
Target Milestone: Firefox 2 beta2 → ---
Whiteboard: [has patch] [change old target milestone]

Comment 26

8 years ago
It seems that this idea has been forgotten, but it's a small and handy feature. That's why I'm bumping it.
Priority: -- → P5
Whiteboard: [fxsearch]
Blocks: 369739

Updated

4 years ago
Rank: 59
You need to log in before you can comment on or make changes to this bug.