639189 - Make search engine alias editable

Status: RESOLVED FIXED

(SeaMonkey :: Search, enhancement)

Description

[Robert Kaiser]

Bug 401417 comment #29, by Neil:
>+  isEditable: function(index, column) { return false; },
Follow-up bug to make the alias editable!


We should also backport this to Firefox.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Proposed fix.

> +  setCellText: function(index, column, value) {
> +    if (column.id != "engineKeyword")
> +      return;
> +    if (gEngineManagerDialog.hasDuplicate(this._engineStore.engines[index], value)) {
> +      setTimeout(() => {
I copied this from Firefox. I don't know why setTimeout() is needed.

Comment 2

[[e-mail]]

Comment on attachment 8546705 [details] [diff] [review]
Patch v1.0 Proposed fix.

>+  hasDuplicate: function(aEngine, aKeyword) {
This is confusing because hasDuplicate updates the keyword if it's not a duplicate. Just call this updateKeyword?

>+    var bduplicate = false;
>+    var eduplicate = false;
>+    var dupName = "";
diff -w next time?

>+      setTimeout(() => {
[The setTimeout is "needed" because the tree XBL code sets the cell text while in the middle of stopping editing, thus we can't start editing just yet. Ideally this would be filed as a toolkit bug. The fat arrow isn't needed here since we're not binding anything, so function() would work.]

>+        document.getElementById("engineList").startEditing(index, column);
I'm not convinced this is a good idea. For instance, if you try to rename "foo.txt" and there already exists a file "bar.txt", when you blur or click to finish the edit, Windows prompts you, but doesn't force you to finish your edit.

>+    <tree id="engineList" flex="1"
Why is the flex not on its own line too?

Comment 3

[Philip Chee]

Attachment: Patch v2.0 fix review issues

>>+  hasDuplicate: function(aEngine, aKeyword) {
> This is confusing because hasDuplicate updates the keyword if it's not
> a duplicate. Just call this updateKeyword?
Done!
 
>>+    var bduplicate = false;
>>+    var eduplicate = false;
>>+    var dupName = "";
> diff -w next time?
Attached is -U8pgw

>>+        document.getElementById("engineList").startEditing(index, column);
> I'm not convinced this is a good idea. For instance, if you try to 
> rename "foo.txt" and there already exists a file "bar.txt", when you blur 
> or click to finish the edit, Windows prompts you, but doesn't force you 
> to finish your edit.
I hope I've understood you. Previous V1 patch doesn't force you to finish your edit. This V2 patch forces edit to finish.

>>+    <tree id="engineList" flex="1"
> Why is the flex not on its own line too?
Fixed.

Comment 4

[[e-mail]]

Comment on attachment 8556003 [details] [diff] [review]
Patch v2.0 fix review issues

>+    if ((isMac && aEvent.keyCode == KeyEvent.DOM_VK_RETURN) ||
>+        (!isMac && aEvent.keyCode == KeyEvent.DOM_VK_F2))
[aEvent.keyCode == (isMac ? KeyEvent.DOM_VK_RETURN : KeyEVent.DOM_VK_F2) perhaps?]

>+                            tree.columns.getNamedColumn("engineKeyword")))
Nit: tree.columns.engineKeyword should work.

>+  isEditable: function(index, column) {
>+    return column.id == "engineKeyword";
>+  },
Nit: if this won't fit on one line, move it up before setCellText.

Comment 5

[Philip Chee]

Pushed to comm-central http://hg.mozilla.org/comm-central/rev/d827294ce603