Closed
Bug 1254762
Opened 9 years ago
Closed 9 years ago
The "Edit Keyword…" button of the Manage Search Engines dialog should be removed
Categories
(Instantbird Graveyard :: Preferences, defect)
Instantbird Graveyard
Preferences
Tracking
(Not tracked)
RESOLVED
FIXED
Instantbird 48
People
(Reporter: florian, Assigned: aybuke.147, Mentored)
Details
(Whiteboard: [good first bug])
Attachments
(1 file, 3 obsolete files)
7.03 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
Steps to reproduce: select some text in the content of a conversation, open the context menu, open the "Search <selection> with…" submenu, and click "Manage Search Engines…".
Alternatively, there's a "Manage Search Engines…" button in the Advanced tab of the preferences window.
In this dialog, the "Edit Keyword…" and the "Keyword" column should be removed. They were there mostly because we copied this dialog from Firefox and didn't want to diverge from the Firefox code... but now that Firefox no longer has this dialog, we should simplify it.
Updated•9 years ago
|
Mentor: florian
Assignee | ||
Comment 1•9 years ago
|
||
Hi, I'm a newbie and interested in this bug. Can you help me about find related file path?
Comment 2•9 years ago
|
||
(In reply to aybukeozdemir from comment #1)
> Hi, I'm a newbie and interested in this bug. Can you help me about find
> related file path?
im/content/engineManager.xul
Assignee | ||
Comment 3•9 years ago
|
||
hi please review the patch :-)
Updated•9 years ago
|
Attachment #8730901 -
Flags: review?(florian)
Comment 4•9 years ago
|
||
Comment on attachment 8730901 [details] [diff] [review]
Deleted keyword button.
Review of attachment 8730901 [details] [diff] [review]:
-----------------------------------------------------------------
Have you checked there are no references to things you are removing here in other source files?
::: im/content/engineManager.xul
@@ -37,5 @@
> <command id="cmd_movedown"
> oncommand="gEngineManagerDialog.bump(-1);"
> disabled="true"/>
> - <command id="cmd_editkeyword"
> - oncommand="gEngineManagerDialog.editKeyword();"
How about also removing the function this calls?
Attachment #8730901 -
Flags: review?(florian) → review-
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to aleth [:aleth] from comment #4)
> Comment on attachment 8730901 [details] [diff] [review]
> Deleted keyword button.
>
> Review of attachment 8730901 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Have you checked there are no references to things you are removing here in
> other source files?
>
> ::: im/content/engineManager.xul
> @@ -37,5 @@
> > <command id="cmd_movedown"
> > oncommand="gEngineManagerDialog.bump(-1);"
> > disabled="true"/>
> > - <command id="cmd_editkeyword"
> > - oncommand="gEngineManagerDialog.editKeyword();"
>
> How about also removing the function this calls?
I thought that; If I remove the button part, its command should remove. But I will fix this and send new patch.
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8730901 [details] [diff] [review]
Deleted keyword button.
Review of attachment 8730901 [details] [diff] [review]:
-----------------------------------------------------------------
::: im/content/engineManager.xul
@@ -58,5 @@
> <treechildren id="engineChildren" flex="1"
> ondragstart="nsDragAndDrop.startDrag(event, gDragObserver);"/>
> <treecols>
> <treecol id="engineName" flex="4" label="&columnLabel.name;"/>
> - <treecol id="engineKeyword" flex="1" label="&columnLabel.keyword;"/>
The JS code used to fill this column should also be removed.
@@ -65,5 @@
> <vbox>
> <spacer flex="1"/>
> - <button id="edit"
> - label="&edit.label;"
> - accesskey="&edit.accesskey;"
These localized strings should be removed from the relevant .dtd file.
Assignee | ||
Comment 7•9 years ago
|
||
I edited engineManager.js and engineManager.dtd files, as you say. I'm glad if you examine the patch :)
Attachment #8731325 -
Flags: review?
Comment 8•9 years ago
|
||
Comment on attachment 8731325 [details] [diff] [review]
removed .js, .dtd and .xul files about edit keyword button.
Review of attachment 8731325 [details] [diff] [review]:
-----------------------------------------------------------------
::: im/content/engineManager.js
@@ +113,1 @@
> var prompt = Cc["@mozilla.org/embedcomp/prompt-service;1"].
This is not valid syntax. Why are you only removing half a function?
@@ +167,1 @@
> noSelection);
What happened here?
Did you test your patch at all? This will give a syntax error.
Attachment #8731325 -
Flags: review? → review-
Assignee | ||
Comment 9•9 years ago
|
||
I fixed and test my patch. (my instantbird screenshot: http://imgur.com/aFgokfm) I would be happy if you review.
Comment 10•9 years ago
|
||
Comment on attachment 8737163 [details] [diff] [review]
1254762_v3.patch
Review of attachment 8737163 [details] [diff] [review]:
-----------------------------------------------------------------
That looks much better :-)
Please double check you haven't left any unused code, e.g.
https://dxr.mozilla.org/comm-central/source/im/content/engineManager.js#466
It looks like your patch is missing a commit message. Here are some instructions to ensure your next patch is ready for checkin:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attachment #8737163 -
Flags: feedback+
Updated•9 years ago
|
Attachment #8731325 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8730901 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
Added commit message and remove 'else if' part for engineKeyword.
Comment 12•9 years ago
|
||
Comment on attachment 8737235 [details] [diff] [review]
Removed the Edit Keyword… button.
Review of attachment 8737235 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, thanks!
Attachment #8737235 -
Flags: review+
Updated•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Attachment #8737163 -
Attachment is obsolete: true
Updated•9 years ago
|
Assignee: nobody → aybuke.147
Status: NEW → ASSIGNED
Comment 13•9 years ago
|
||
https://hg.mozilla.org/comm-central/rev/ea2aa3bf1ffa4a0a107d51747e26fcc7bdac9d01
Bug 1254762 - Remove the Edit Keyword button of the Manage Search Engines dialog. r=aleth
Updated•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 48
Reporter | ||
Comment 14•9 years ago
|
||
Is engineManager.properties now unused?
You need to log in
before you can comment on or make changes to this bug.
Description
•