Closed
Bug 1332899
Opened 4 years ago
Closed 4 years ago
Port Bug 1329926 and remove getURIForKeyword API from SeaMonkey.
Categories
(SeaMonkey :: Bookmarks & History, defect)
Tracking
(seamonkey2.49esr fixed, seamonkey2.50 fixed, seamonkey2.51 fixed, seamonkey2.52 fixed)
RESOLVED
FIXED
seamonkey2.52
People
(Reporter: frg, Assigned: frg)
Details
Attachments
(2 files, 1 obsolete file)
6.87 KB,
patch
|
iann_bugzilla
:
review+
iann_bugzilla
:
approval-comm-aurora+
iann_bugzilla
:
approval-comm-beta+
iann_bugzilla
:
approval-comm-release+
|
Details | Diff | Splinter Review |
6.81 KB,
patch
|
frg
:
review+
|
Details | Diff | Splinter Review |
Bug 1329926 removed getURIForKeyword. In SeaMonkey it is still used in suite/common/search/engineManager.js
![]() |
Assignee | |
Comment 1•4 years ago
|
||
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.
![]() |
||
Comment 2•4 years ago
|
||
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-
![]() |
||
Comment 3•4 years ago
|
||
> 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
![]() |
Assignee | |
Comment 4•4 years ago
|
||
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)
![]() |
Assignee | |
Updated•4 years ago
|
status-seamonkey2.51:
--- → affected
![]() |
Assignee | |
Comment 5•4 years ago
|
||
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?
![]() |
Assignee | |
Updated•4 years ago
|
status-seamonkey2.52:
--- → affected
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+
![]() |
Assignee | |
Comment 7•4 years ago
|
||
Patch with issues addressed. r+ from IanN carried forward.
Attachment #8849631 -
Flags: review+
![]() |
Assignee | |
Updated•4 years ago
|
Attachment #8833788 -
Flags: review?(philip.chee)
![]() |
Assignee | |
Comment 8•4 years ago
|
||
https://hg.mozilla.org/comm-central/rev/8112dc270fcad461fa471ae2387a40a95806d3a3 https://hg.mozilla.org/releases/comm-aurora/rev/7abca4531ba32e48d9a94a3cbd0dee0f2a545256 https://hg.mozilla.org/releases/comm-beta/rev/7c268ed6732e420a3bcd5f38d3fa807600b1ad6d https://hg.mozilla.org/releases/comm-release/rev/a6e7681a0b0433fa027afd36606ddfad82756ef0 https://hg.mozilla.org/releases/comm-esr52/rev/91edc5e459cd540184c29fbae38d234776fc08ab
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.52
![]() |
Assignee | |
Comment 9•4 years ago
|
||
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 10•4 years ago
|
||
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+
![]() |
Assignee | |
Comment 11•4 years ago
|
||
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.
Description
•