Last Comment Bug 325777 - "Search messages" window has mislabeled button - "File" should be "Move"
: "Search messages" window has mislabeled button - "File" should be "Move"
Status: RESOLVED FIXED
[good first bug]
: uiwanted
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 22.0
Assigned To: sakshi
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-03 10:44 PST by Neil Parks
Modified: 2014-07-27 05:56 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Bug fix (1.15 KB, patch)
2009-12-13 17:32 PST, Mina
bwinton: ui‑review-
iann_bugzilla: feedback-
Details | Diff | Splinter Review
New patch (4.94 KB, patch)
2013-03-09 08:59 PST, sakshi
no flags Details | Diff | Splinter Review
New patch (7.68 KB, patch)
2013-03-15 05:54 PDT, sakshi
no flags Details | Diff | Splinter Review
Patch 3 (7.68 KB, patch)
2013-03-15 06:03 PDT, sakshi
bwinton: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
Patch4 (7.68 KB, patch)
2013-03-25 04:17 PDT, sakshi
no flags Details | Diff | Splinter Review
patch5 (7.68 KB, patch)
2013-03-25 06:07 PDT, sakshi
sakshi.april5: review+
iann_bugzilla: review-
sakshi.april5: ui‑review+
Details | Diff | Splinter Review
Patch6 (7.68 KB, patch)
2013-03-31 04:25 PDT, sakshi
iann_bugzilla: review+
sakshi.april5: ui‑review+
Details | Diff | Splinter Review

Description Neil Parks 2006-02-03 10:44:19 PST
User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Thunderbird 1.5

There are five buttons across the bottom of the "Search Messages" window, labeled "Open", "File", "Delete", "Open Msg Folder", "Save As Search Folder".

I believe the "File" button is mislabeled.  Its function is to move the selected msg from its current folder (identified in the Location column) to a different folder.  Therefore, it should logically be labeled "Move Message".



Reproducible: Always

Steps to Reproduce:
Comment 1 Mike Cowperthwaite 2006-02-04 08:24:42 PST
Sounds reasonable.  However, a change like this is nowhere near important 
enough to request blocking for.
Comment 2 Neil Parks 2006-09-14 09:09:27 PDT
If I "customize" the message window while reading a message, there is an optional button labeled "File" that can be added to the toolbar.  

The same problem applies.  Its purpose is to Move the message to a different folder, Therefore its name should be "Move".  

Comment 3 Mina 2009-12-13 17:32:38 PST
Created attachment 417416 [details] [diff] [review]
Bug fix
Comment 4 Mark Banner (:standard8) 2009-12-14 06:34:54 PST
Comment on attachment 417416 [details] [diff] [review]
Bug fix

SeaMonkey specific review rules can be found here:

http://www.seamonkey-project.org/dev/review-and-flags
Comment 5 :aceman 2011-11-07 12:54:50 PST
If there is "Open" and "Delete" then the "File" should probably just be "Move to" (as in the context menu in the main message list).
But also note there is a "File here" when you navigate the folder tree. That is the same as when using the context menu.
The product is set to Thunderbird but the path is for Seamonkey?

There is also a File icon available to be added to the main TB toolbar.
Filters again use "Move to" and "Copy to".
So what is the proper name for this operation?
Comment 6 Blake Winton (:bwinton) (:☕️) 2012-07-25 09:03:02 PDT
Comment on attachment 417416 [details] [diff] [review]
Bug fix

>+++ b/suite/locales/en-US/chrome/mailnews/SearchDialog.dtd	Mon Dec 14 03:29:07 2009 +0200
>@@ -10,18 +10,18 @@
> <!ENTITY openButton.label            "Open">
> <!ENTITY openButton.accesskey        "O">
> <!ENTITY deleteButton.label          "Delete">
> <!ENTITY deleteButton.accesskey      "D">
> <!ENTITY searchDialogTitle.label     "Search Messages">
> <!ENTITY results.label               "Results">
> <!ENTITY fileHereMenu.label          "File Here">
> <!ENTITY fileHereMenu.accesskey      "r">
>-<!ENTITY fileButton.label            "File">
>-<!ENTITY fileButton.accesskey        "F">
>+<!ENTITY fileButton.label            "Move Message">
>+<!ENTITY fileButton.accesskey        "M">

I'm pretty sure you have to change the ids when you change the text.

Also, Since we have "Open" and "Delete" (instead of "Open Message" and "Delete Message"), we should go with "Move" instead of "Move Message".

Finally, the fileHereMenu label and access key should probably change to moveHere, to be consistent.

So, ui-r-, so that I can double-check the results of all those changes.

Thanks,
Blake.
Comment 7 :aceman 2012-07-25 10:24:34 PDT
Mina, can you continue on the patch?
Comment 8 Ian Neal (Away until 7th Aug) 2012-07-29 03:08:20 PDT
Comment on attachment 417416 [details] [diff] [review]
Bug fix

As previously mentioned, need to change the fileHereMenu.* entities too and, because you are changing the strings, you will need to rename the entities.
This means changing xul/js files too.

You will need to change both TB and Suite:
http://mxr.mozilla.org/comm-central/source/mail/base/content/SearchDialog.xul
http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/SearchDialog.dtd
http://mxr.mozilla.org/comm-central/source/suite/mailnews/search/SearchDialog.xul
http://mxr.mozilla.org/comm-central/source/suite/locales/en-US/chrome/mailnews/SearchDialog.dtd

The main display window also has File instead of Move too, so there are entities in messenger.dtd/mailWindowOverlay.xul that will need changing.
The compose window has fileHereMenu (though this might have to be changed to Copy Here rather than Move Here - maybe Blake can comment), so there are entities in messengercompose.dtd/messengercompose.xul that will need changing.

Do we need to change things like "showFileHereLabel" or various xul element IDs?

Either way r- for the moment.
Comment 9 Ludovic Hirlimann [:Usul] 2012-09-03 08:50:23 PDT
Mina ?
Comment 10 sakshi 2013-03-08 05:11:36 PST
Hello,

I would like to fix this bug.
Comment 11 Blake Winton (:bwinton) (:☕️) 2013-03-08 07:05:47 PST
Awesome!  I've assigned the bug to you, and look forward to your patch.  :)

Please feel free to email me if you need any help…

Thanks,
Blake.
Comment 12 sakshi 2013-03-09 08:59:56 PST
Created attachment 723093 [details] [diff] [review]
New patch

who should I set as reviewer?
Comment 13 :aceman 2013-03-09 10:25:50 PST
Try bwinton.
Comment 14 sakshi 2013-03-15 04:51:40 PDT
Hello,

Any updates on the submitted patch?
Comment 15 :aceman 2013-03-15 05:17:40 PDT
I think it looks good, you even covered both TB and Seamonkey.
But I think you have not addressed comment 8. Can you look at that?
Comment 16 sakshi 2013-03-15 05:54:15 PDT
Created attachment 725362 [details] [diff] [review]
New patch

Oh ya. I guess I missed that point.
Comment 17 :aceman 2013-03-15 05:57:20 PDT
What is "Mile" ? :)
+<!ENTITY moveButton.label            "Mile">
Comment 18 sakshi 2013-03-15 06:03:08 PDT
Created attachment 725366 [details] [diff] [review]
Patch 3
Comment 19 :aceman 2013-03-15 06:10:49 PDT
Maybe you could number your patches in some way (e.g. patch 1, patch 2) so that we can easily reference them. Also in reviews it will be visible, which version was reviewed and what were the problems then (see e.g. comment 8, line 2).
Comment 20 sakshi 2013-03-15 06:12:07 PDT
Ya sure. I will do that.
Comment 21 rsx11m 2013-03-20 08:47:49 PDT
Note that there is also a "File" button in the toolbar customization palette for both the Thunderbird 3-pane and SeaMonkey's Mail & News windows. Thus, if changed for the Search dialog, this should also be changed in the toolbar buttons to make it consistent. I don't expect this to be covered here, but we should keep it in mind for follow-up bugs if this hasn't been filed yet for either application.
Comment 22 rsx11m 2013-03-22 08:22:27 PDT
(In reply to Ian Neal from comment #8)
> As previously mentioned, need to change the fileHereMenu.* entities too and,
> because you are changing the strings, you will need to rename the entities.

When are those actually supposed to show up? I see them when using File from the Search window, only for an account but not for Local Folders. It's also disabled for me in those instances where I can see it.

> The main display window also has File instead of Move too, so there are
> entities in messenger.dtd/mailWindowOverlay.xul that will need changing.

I'd suggest to move those into separate bugs as the File button involves other changes as well (specifically, CSS and Help changes, and Thunderbird would need a different icon as the floppy metaphor doesn't quite fit the Move action).

> The compose window has fileHereMenu (though this might have to be changed to
> Copy Here rather than Move Here - maybe Blake can comment), so there are
> entities in messengercompose.dtd/messengercompose.xul that will need changing.

I don't see this showing up in neither Thunderbird nor SeaMonkey when using the File button in the primary toolbar or Options > Send a Copy to from the message composition window.
Comment 23 Blake Winton (:bwinton) (:☕️) 2013-03-24 19:34:10 PDT
Comment on attachment 725366 [details] [diff] [review]
Patch 3

>+++ b/mail/locales/en-US/chrome/messenger/SearchDialog.dtd
>@@ -12,20 +12,20 @@
>-<!ENTITY fileHereMenu.label          "File Here">
>-<!ENTITY fileHereMenu.accesskey      "F">
>-<!ENTITY fileButton.label            "File">
>-<!ENTITY fileButton.accesskey        "i">
>+<!ENTITY moveHereMenu.label          "Move Here">
>+<!ENTITY moveHereMenu.accesskey      "h">
>+<!ENTITY moveButton.label            "Move">

I think this should be "Move To", not just "Move".  (I think I might have said "Move" in a previous comment, and if so, I'm sorry.)

Other than that, it looks good to me, so r=me, with that change made.  (And I'll give the official ui-r=me, while I'm here. ;)

Thanks,
Blake.
Comment 24 sakshi 2013-03-25 04:17:17 PDT
Created attachment 728918 [details] [diff] [review]
Patch4
Comment 25 rsx11m 2013-03-25 05:52:42 PDT
Comment on attachment 728918 [details] [diff] [review]
Patch4

Sakshi, since your patch affects both mail/ and suite/ modules, you will also need review from both Thunderbird and SeaMonkey, thus I'm adding back that request.

(In reply to Blake Winton (:bwinton) from comment #23)
> Other than that, it looks good to me, so r=me, with that change made.  (And
> I'll give the official ui-r=me, while I'm here. ;)

This means that you can carry forward review+ and ui-review+ which you received for attachment 725366 [details] [diff] [review] from Blake to the new patch, thus you now only have to wait for Ian's review on the SeaMonkey side.

I found a nit though which you may want to address:
>+++ b/mail/locales/en-US/chrome/messenger/SearchDialog.dtd
>+<!ENTITY moveButton.label            "Move To">
>+<!ENTITY moveButton.accesskey        "m">

Make it "M" for the accesskey definition. While "m" works, it is less efficient (the first run makes a case-sensitive match, only then a case-insensitive search). It's fine on the SeaMonkey side:

>+++ b/suite/locales/en-US/chrome/mailnews/SearchDialog.dtd
>+<!ENTITY moveButton.label            "Move To">
>+<!ENTITY moveButton.accesskey        "M">
Comment 26 rsx11m 2013-03-25 05:55:51 PDT
(In reply to rsx11m from comment #25)
> This means that you can carry forward review+ and ui-review+ which you
> received for attachment 725366 [details] [diff] [review] from Blake to the new patch

You should be able to set review[+] and ui-review[+] yourself when uploading a patch, then add Ian in the "addl. review" field. If you don't have the necessary permissions, please say so and someone should give them to you.
Comment 27 sakshi 2013-03-25 06:02:30 PDT
I don't think I have the permission to do so.
Comment 28 sakshi 2013-03-25 06:07:25 PDT
Created attachment 728937 [details] [diff] [review]
patch5
Comment 29 rsx11m 2013-03-25 06:47:26 PDT
(In reply to sakshi from comment #27)
> I don't think I have the permission to do so.

Sakshi, Wayne says that you should have editbugs privileges already. So, can you try setting review and ui-review to [+] and request addl. review from Ian again?
Comment 30 sakshi 2013-03-25 06:52:44 PDT
Yes, it works.
Comment 31 rsx11m 2013-03-25 06:53:23 PDT
Ok, this looks good now.  :-)
Comment 32 Ian Neal (Away until 7th Aug) 2013-03-30 15:58:22 PDT
Comment on attachment 728937 [details] [diff] [review]
patch5

>+++ b/mail/locales/en-US/chrome/messenger/SearchDialog.dtd
>@@ -12,20 +12,20 @@
> <!ENTITY resetButton.label           "Clear">
> <!ENTITY resetButton.accesskey       "C">
> <!ENTITY openButton.label            "Open">
> <!ENTITY openButton.accesskey        "n">
> <!ENTITY deleteButton.label          "Delete">
> <!ENTITY deleteButton.accesskey      "D">
> <!ENTITY searchDialogTitle.label     "Search Messages">
> <!ENTITY results.label               "Results">
>+<!ENTITY moveHereMenu.label          "Move Here">
>+<!ENTITY moveHereMenu.accesskey      "h">
Should be "H"

>+<!ENTITY moveButton.label            "Move To">
>+<!ENTITY moveButton.accesskey        "M">
You cannot use "M" here as it will clash with matchAllMsgs.accesskey from searchTermOverlay.dtd, perhaps "T" would be suitable?

>+++ b/suite/locales/en-US/chrome/mailnews/SearchDialog.dtd
>@@ -12,20 +12,20 @@
> <!ENTITY resetButton.label           "Clear">
> <!ENTITY resetButton.accesskey       "C">
> <!ENTITY openButton.label            "Open">
> <!ENTITY openButton.accesskey        "O">
> <!ENTITY deleteButton.label          "Delete">
> <!ENTITY deleteButton.accesskey      "D">
> <!ENTITY searchDialogTitle.label     "Search Messages">
> <!ENTITY results.label               "Results">
>+<!ENTITY moveHereMenu.label          "Move Here">
>+<!ENTITY moveHereMenu.accesskey      "h">
This should be "H", I don't think popup menus use the Alt key so it won't clash with the "H" for the Help button.

>+<!ENTITY moveButton.label            "Move To">
>+<!ENTITY moveButton.accesskey        "M">
You cannot use "M" here as it will clash with matchAll.accesskey from searchTermOverlay.dtd, perhaps "T" would be suitable as the moveButton and matchAllMsgs radio never appear on the same window?

r- as I would like to see the new patch.
Comment 33 sakshi 2013-03-31 04:25:49 PDT
Created attachment 731631 [details] [diff] [review]
Patch6
Comment 34 rsx11m 2013-04-01 04:21:52 PDT
Setting checkin-needed on sakshi's behalf so that this patch still lands before tomorrow's scheduled merge.
Comment 35 Ryan VanderMeulen [:RyanVM] 2013-04-01 07:29:16 PDT
https://hg.mozilla.org/comm-central/rev/7eed919c008a

Note You need to log in before you can comment on or make changes to this bug.