The "Edit Keyword…" button of the Manage Search Engines dialog should be removed

RESOLVED FIXED in Instantbird 48

Status

defect
--
minor
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: florian, Assigned: aybuke.147, Mentored)

Tracking

trunk
Instantbird 48

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 3 obsolete attachments)

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.
Mentor: florian
Hi, I'm a newbie and interested in this bug. Can you help me about find related file path?
(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
Posted patch Deleted keyword button. (obsolete) — Splinter Review
hi  please review the patch :-)
Attachment #8730901 - Flags: review?(florian)
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-
(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.
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.
I edited engineManager.js and engineManager.dtd files, as you say. I'm glad if you examine the patch :)
Attachment #8731325 - Flags: review?
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-
Posted patch 1254762_v3.patch (obsolete) — Splinter Review
I fixed and test my patch. (my instantbird screenshot: http://imgur.com/aFgokfm) I would be happy if you review.
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+
Attachment #8731325 - Attachment is obsolete: true
Attachment #8730901 - Attachment is obsolete: true
Added commit message and remove 'else if' part for engineKeyword.
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+
Keywords: checkin-needed
Attachment #8737163 - Attachment is obsolete: true
Assignee: nobody → aybuke.147
Status: NEW → ASSIGNED
https://hg.mozilla.org/comm-central/rev/ea2aa3bf1ffa4a0a107d51747e26fcc7bdac9d01
Bug 1254762 - Remove the Edit Keyword button of the Manage Search Engines dialog. r=aleth
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 48
Is engineManager.properties now unused?
You need to log in before you can comment on or make changes to this bug.