:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Description • 18 years ago

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

	Joe Hughes
	Comment 1 • 18 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 2 • 18 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 3 • 18 years ago

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).

	Régis Caspar
	Comment 4 • 18 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.

	Joey Minta
	Comment 5 • 18 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 6 • 18 years ago

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?

	Joey Minta
	Comment 7 • 18 years ago

(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.

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 8 • 18 years ago

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.

	Joey Minta
	Comment 9 • 18 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).

	Joey Minta
	Comment 10 • 18 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 11 • 18 years ago

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.

	Joey Minta
	Comment 12 • 18 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 13 • 18 years ago

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).

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 14 • 18 years ago

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

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 15 • 18 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 16 • 18 years ago

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

Yeah, this is missing a lot of stuff.

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 17 • 18 years ago

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.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 18 • 18 years ago

Convincing arguments, Gavin!

	Jo Hermans
	Comment 19 • 18 years ago

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

	Joey Minta
	Comment 20 • 17 years ago

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

	Andrés Delfino
	Comment 21 • 16 years ago

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

	Mike Connor [:mconnor]
	Comment 22 • 16 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 23 • 15 years ago

Gavin, is qawanted still needed?

	Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)
	Comment 24 • 15 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 25 • 15 years ago

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

	philip_hoflack
	Comment 26 • 13 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.