Closed Bug 1332899 Opened 3 years ago Closed 2 years ago

Port Bug 1329926 and remove getURIForKeyword API from SeaMonkey.

Categories

(SeaMonkey :: Bookmarks & History, defect)

SeaMonkey 2.50 Branch
defect
Not set

Tracking

(seamonkey2.49esr fixed, seamonkey2.50 fixed, seamonkey2.51 fixed, seamonkey2.52 fixed)

RESOLVED FIXED
seamonkey2.52
Tracking Status
seamonkey2.49esr --- fixed
seamonkey2.50 --- fixed
seamonkey2.51 --- fixed
seamonkey2.52 --- fixed

People

(Reporter: frg, Assigned: frg)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1329926 removed getURIForKeyword.

In SeaMonkey it is still used in suite/common/search/engineManager.js
Attached patch 1332899-getURIForKeyword.patch (obsolete) — Splinter Review
Not 100% happy with the patch. 

The new API is asynchronous and so won't return a result. Using .then didn't work either very well so I just removed the while loop for the dialog. Updates work fine but in case the user selects an existing keyword an error message dialog will be shown and the edit dialog dismissed where he/she before had the chance to change the value. But I think in this case the impact is neglectable.

The corresponding routine in browser is editKeyword in mozilla/browser/components/preferences/in-content/search.js.
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Attachment #8829243 - Flags: review?(philip.chee)
Comment on attachment 8829243 [details] [diff] [review]
1332899-getURIForKeyword.patch

The reason we have updateKeyword as well as editKeyword is because we can edit a keyword via two paths. One via the "Edit Keyword" button. And one by double clicking on the keyword column at the appropriate row. Neil insisted on this which was a PITA. By the way you missed to call to updateKeyword in setCellText.

[1] What we can do is to have only one edit path like Firefox. Remove the Edit Keyword button and the updateKeyword function. If we do this we might add some help text telling the user that they can double click on the keyword field to edit or add a keyword. Perhaps a tooltip. Or we could make the "Edit Keyword" button just call setCellText() with the right parameters.

> Not 100% happy with the patch. 

> The new API is asynchronous and so won't return a result. Using .then didn't
> work either very well so I just removed the while loop for the dialog.
> Updates work fine but in case the user selects an existing keyword an error
> message dialog will be shown and the edit dialog dismissed where he/she
> before had the chance to change the value. But I think in this case the
> impact is neglectable.

[2] Or you could just convert this SQL to JavaScript:
https://hg.mozilla.org/mozilla-central/rev/7feb3cffb4a2#l2.12
(I hear you're a whiz at SQL ;) )

> The corresponding routine in browser is editKeyword in
> mozilla/browser/components/preferences/in-content/search.js.

I'd try [1] first with the Edit button calling setCellText() directly.
Attachment #8829243 - Flags: review?(philip.chee) → review-
> I'd try [1] first with the Edit button calling setCellText() directly.
Well actually we can just follow Firefox as in:
https://hg.mozilla.org/mozilla-central/rev/75afd4419cd3#l4.11
I wouldn't have thought in a 100 years that the field in the table is editable. 

> +    let tree = document.getElementById("engineList");
> +    let column = tree.columns.getColumnFor(document.getElementById("engineKeyword"));
> +    document.getElementById("engineList").startEditing(index, column);

Is there an easier way to get the column?
Attachment #8829243 - Attachment is obsolete: true
Attachment #8833788 - Flags: review?(philip.chee)
Comment on attachment 8833788 [details] [diff] [review]
1332899-getURIForKeyword-V2.patch

[Approval Request Comment]
Regression caused by (bug #): 1329926
User impact if declined: keyword can not be edited.
Testing completed (on m-c, etc.): c-c c-a
Risk to taking this patch (and alternatives if risky): broken already.
String changes made by this patch: none

Ratty seems to be currently unavailable. IanN can you take a look and review if ok.
Attachment #8833788 - Flags: review?(iann_bugzilla)
Attachment #8833788 - Flags: approval-comm-beta?
Attachment #8833788 - Flags: approval-comm-aurora?
Comment on attachment 8833788 [details] [diff] [review]
1332899-getURIForKeyword-V2.patch

>+++ b/suite/common/search/engineManager.js
>+  selectEditKeyword: function engineManager_selectEditKeyword() {
>+    let selectedEngine = gEngineView.selectedEngine;
>     if (!selectedEngine)
>       return;
Can we not use gEngineView.selectedIndex instead and test it is not -1?

>+    let index = gEngineView._engineStore._getIndexForEngine(selectedEngine);
>+    let tree = document.getElementById("engineList");
>+    let column = tree.columns.getColumnFor(document.getElementById("engineKeyword"));
>+    document.getElementById("engineList").startEditing(index, column);
Can this last one be:
tree.startEditing(index, column);

r/a=me with those addressed/answered
Attachment #8833788 - Flags: review?(iann_bugzilla)
Attachment #8833788 - Flags: review+
Attachment #8833788 - Flags: approval-comm-beta?
Attachment #8833788 - Flags: approval-comm-beta+
Attachment #8833788 - Flags: approval-comm-aurora?
Attachment #8833788 - Flags: approval-comm-aurora+
Patch with issues addressed. r+ from IanN carried forward.
Attachment #8849631 - Flags: review+
Attachment #8833788 - Flags: review?(philip.chee)
Comment on attachment 8833788 [details] [diff] [review]
1332899-getURIForKeyword-V2.patch

Drat accidently pushed this to c-r and c-esr52 too without approval. But it will work. Please let me know if I should back it out or set approve. Sorry. 

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: none still works in 2.49
Testing completed (on m-c, etc.): c-b to c-c
Risk to taking this patch (and alternatives if risky): none
String changes made by this patch: none
Attachment #8833788 - Flags: approval-comm-release?
Comment on attachment 8833788 [details] [diff] [review]
1332899-getURIForKeyword-V2.patch

a=me assuming the code the patch relies on is on m-r/m-esr
Attachment #8833788 - Flags: approval-comm-release? → approval-comm-release+
Thanks. The api removed for Gecko 53 was very old. Everything the patch relies on was already in place. Retest with 2.49 was successful.
You need to log in before you can comment on or make changes to this bug.