Closed Bug 325777 Opened 18 years ago Closed 11 years ago

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

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 22.0

People

(Reporter: nparks, Assigned: sakshi.april5)

Details

(Keywords: uiwanted, Whiteboard: [good first bug])

Attachments

(1 file, 6 obsolete files)

7.68 KB, patch
iannbugzilla
: review+
sakshi.april5
: ui-review+
Details | Diff | Splinter Review
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:
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Sounds reasonable.  However, a change like this is nowhere near important 
enough to request blocking for.
Severity: normal → trivial
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Version: unspecified → Trunk
QA Contact: front-end
Summary: "Search messages" window has mislabeled button → "Search messages" window has mislabeled button - "File" should be "Move"
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".  

Assignee: mscott → nobody
Attached patch Bug fix (obsolete) — Splinter Review
Attachment #417416 - Flags: review?(felsayedmeawad)
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
Attachment #417416 - Flags: review?(felsayedmeawad)
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?
Keywords: uiwanted
Whiteboard: [good first bug]
Attachment #417416 - Flags: ui-review?(bwinton)
Attachment #417416 - Flags: feedback?(iann_bugzilla)
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.
Attachment #417416 - Flags: ui-review?(bwinton) → ui-review-
Mina, can you continue on the patch?
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.
Attachment #417416 - Flags: feedback?(iann_bugzilla) → feedback-
Mina ?
Whiteboard: [good first bug] → [good first bug][patchlove]
Hello,

I would like to fix this bug.
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.
Assignee: nobody → sakshi.april5
Status: NEW → ASSIGNED
Attached patch New patch (obsolete) — Splinter Review
who should I set as reviewer?
Try bwinton.
Attachment #723093 - Flags: review?(bwinton)
Hello,

Any updates on the submitted patch?
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?
Attached patch New patch (obsolete) — Splinter Review
Oh ya. I guess I missed that point.
Attachment #723093 - Attachment is obsolete: true
Attachment #723093 - Flags: review?(bwinton)
Attachment #725362 - Flags: review?(bwinton)
What is "Mile" ? :)
+<!ENTITY moveButton.label            "Mile">
Attached patch Patch 3 (obsolete) — Splinter Review
Attachment #725362 - Attachment is obsolete: true
Attachment #725362 - Flags: review?(bwinton)
Attachment #725366 - Flags: review?(bwinton)
Attachment #725366 - Flags: feedback?(iann_bugzilla)
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).
Ya sure. I will do that.
Attachment #725366 - Attachment description: New patch → Patch 3
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.
(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 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.
Attachment #725366 - Flags: ui-review+
Attachment #725366 - Flags: review?(bwinton)
Attachment #725366 - Flags: review+
Attached patch Patch4 (obsolete) — Splinter Review
Attachment #725366 - Attachment is obsolete: true
Attachment #725366 - Flags: feedback?(iann_bugzilla)
Attachment #728918 - Flags: review?(bwinton)
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">
Attachment #728918 - Flags: review?(iann_bugzilla)
(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.
I don't think I have the permission to do so.
Attached patch patch5 (obsolete) — Splinter Review
Attachment #728918 - Attachment is obsolete: true
Attachment #728918 - Flags: review?(iann_bugzilla)
Attachment #728918 - Flags: review?(bwinton)
Attachment #728937 - Flags: review?(bwinton)
(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?
Attachment #728937 - Flags: ui-review?(sakshi.april5)
Attachment #728937 - Flags: review?(sakshi.april5)
Attachment #728937 - Flags: review?(iann_bugzilla)
Attachment #728937 - Flags: review?(bwinton)
Attachment #728937 - Flags: ui-review?(sakshi.april5)
Attachment #728937 - Flags: ui-review+
Attachment #728937 - Flags: review?(sakshi.april5)
Attachment #728937 - Flags: review+
Yes, it works.
Ok, this looks good now.  :-)
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.
Attachment #728937 - Flags: review?(iann_bugzilla) → review-
Attached patch Patch6Splinter Review
Attachment #728937 - Attachment is obsolete: true
Attachment #731631 - Flags: ui-review+
Attachment #731631 - Flags: review?(iann_bugzilla)
Attachment #731631 - Flags: review?(iann_bugzilla) → review+
Setting checkin-needed on sakshi's behalf so that this patch still lands before tomorrow's scheduled merge.
Keywords: checkin-needed
Attachment #417416 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/7eed919c008a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0
Whiteboard: [good first bug][patchlove] → [good first bug]
You need to log in before you can comment on or make changes to this bug.