The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Instantbird 48

Status

Instantbird
Preferences
--
minor
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: florian, Assigned: aybukeozdemir, Mentored)

Tracking

trunk
Instantbird 48

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

a year ago
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

a year ago
Mentor: florian@queze.net
(Assignee)

Comment 1

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

Comment 2

a year 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

a year ago
Created attachment 8730901 [details] [diff] [review]
Deleted keyword button.

hi  please review the patch :-)
Attachment #8730901 - Flags: review?(florian)

Comment 4

a year 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

a year 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

a year 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

a year ago
Created attachment 8731325 [details] [diff] [review]
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 :)
Attachment #8731325 - Flags: review?

Comment 8

a year 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

a year ago
Created attachment 8737163 [details] [diff] [review]
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

a year 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

a year ago
Attachment #8731325 - Attachment is obsolete: true

Updated

a year ago
Attachment #8730901 - Attachment is obsolete: true
(Assignee)

Comment 11

a year ago
Created attachment 8737235 [details] [diff] [review]
Removed the Edit Keyword… button.

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

Comment 12

a year 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

a year ago
Keywords: checkin-needed

Updated

a year ago
Attachment #8737163 - Attachment is obsolete: true

Updated

a year ago
Assignee: nobody → aybuke.147
Status: NEW → ASSIGNED

Comment 13

a year 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

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 48
(Reporter)

Comment 14

a year ago
Is engineManager.properties now unused?
You need to log in before you can comment on or make changes to this bug.