325777 - "Search messages" window has mislabeled button - "File" should be "Move"

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Neil Parks]

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]

Sounds reasonable.  However, a change like this is nowhere near important 
enough to request blocking for.

Comment 2

[Neil Parks]

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]

Attachment: Bug fix

Comment 4

[Mark Banner (:standard8)]

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]

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) (:☕️)]

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]

Mina, can you continue on the patch?

Comment 8

[Ian Neal]

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]]

Mina ?

Comment 10

[sakshi]

Hello,

I would like to fix this bug.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

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]

Attachment: New patch

who should I set as reviewer?

Comment 13

[:aceman]

Try bwinton.

Comment 14

[sakshi]

Hello,

Any updates on the submitted patch?

Comment 15

[:aceman]

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]

Attachment: New patch

Oh ya. I guess I missed that point.

Comment 17

[:aceman]

What is "Mile" ? :)
+<!ENTITY moveButton.label            "Mile">

Comment 18

[sakshi]

Attachment: Patch 3

Comment 19

[:aceman]

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]

Ya sure. I will do that.

Comment 21

[rsx11m]

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]

(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) (:☕️)]

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]

Attachment: Patch4

Comment 25

[rsx11m]

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]

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

I don't think I have the permission to do so.

Comment 28

[sakshi]

Attachment: patch5

Comment 29

[rsx11m]

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

Yes, it works.

Comment 31

[rsx11m]

Ok, this looks good now.  :-)

Comment 32

[Ian Neal]

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]

Attachment: Patch6

Comment 34

[rsx11m]

Setting checkin-needed on sakshi's behalf so that this patch still lands before tomorrow's scheduled merge.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/7eed919c008a