Closed Bug 639189 Opened 10 years ago Closed 6 years ago

Make search engine alias editable

Categories

(SeaMonkey :: Search, enhancement)

enhancement
Not set
normal

Tracking

(seamonkey2.35 fixed)

RESOLVED FIXED
seamonkey2.35
Tracking Status
seamonkey2.35 --- fixed

People

(Reporter: kairo, Assigned: philip.chee)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch v1.0 Proposed fix. (obsolete) — Splinter Review
> +  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.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #8546705 - Flags: review?(neil)
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?
>>+  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.
Attachment #8546705 - Attachment is obsolete: true
Attachment #8546705 - Flags: review?(neil)
Attachment #8556003 - Flags: review?(neil)
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.
Attachment #8556003 - Flags: review?(neil) → review+
Pushed to comm-central http://hg.mozilla.org/comm-central/rev/d827294ce603
Severity: normal → enhancement
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.35
Duplicate of this bug: 640418
You need to log in before you can comment on or make changes to this bug.