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

Status: RESOLVED FIXED

(Instantbird Graveyard :: Preferences, defect)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[aybukeozdemir]

Hi, I'm a newbie and interested in this bug. Can you help me about find related file path?

Comment 2

[aleth [:aleth]]

(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

Comment 3

[aybukeozdemir]

Attachment: Deleted keyword button.

hi  please review the patch :-)

Comment 4

[aleth [:aleth]]

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?

Comment 5

[aybukeozdemir]

(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 6

[Florian Quèze [:florian]]

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.

Comment 7

[aybukeozdemir]

Attachment: removed .js, .dtd and .xul files about edit keyword button.

I edited engineManager.js and engineManager.dtd files, as you say. I'm glad if you examine the patch :)

Comment 8

[aleth [:aleth]]

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.

Comment 9

[aybukeozdemir]

Attachment: 1254762_v3.patch

I fixed and test my patch. (my instantbird screenshot: http://imgur.com/aFgokfm) I would be happy if you review.

Comment 10

[aleth [:aleth]]

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

Comment 11

[aybukeozdemir]

Attachment: Removed the Edit Keyword… button.

Added commit message and remove 'else if' part for engineKeyword.

Comment 12

[aleth [:aleth]]

Comment on attachment 8737235 [details] [diff] [review]
Removed the Edit Keyword… button.

Review of attachment 8737235 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 13

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/ea2aa3bf1ffa4a0a107d51747e26fcc7bdac9d01
Bug 1254762 - Remove the Edit Keyword button of the Manage Search Engines dialog. r=aleth

Comment 14

[Florian Quèze [:florian]]

Is engineManager.properties now unused?