520214 - "remove attachment" is inconsistent and doesn't work with multi-select

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

+++ This bug was initially created as a clone of Bug #296781 +++

Bug 296781 was rushed in IMO. "Remove Attachment" in the context menu doesn't is inconsistent and doesn't work well for multi-select. It should just be "Remove".

I guess we'll have to live with it for tb3, but filing it now either way.

Screenshot http://imagebin.ca/view/INgUYm77.html

Comment 1

[Phil Ringnalda (:philor)]

Comment on attachment 404291 [details] [diff] [review]
proposed fix

Since we've got all the time in the world to change it now, how about a change in entity name that reflects the change in meaning, like removeOneOrMoreAttachments (since there's plenty of room and you don't have to type it many times)?

I'd suggest removeAttachment(s), but I don't think you can use parentheses in entity names :)

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 404291 [details] [diff] [review]
proposed fix

just remove would work as well though I prefer remove attachment as it's clearer.

perhaps we should be using the plural l10n code for this.

Comment 3

[Magnus Melin [:mkmelin]]

How's that clearer?
All other context menus do not (rightfully) say what they are for: mails, folders, accounts, toolbars. It's called a *context* menu after all.

Comment 4

[Jae-Seong Lee-Russo]

Attachment: Use PluralForm, 1

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #3)
> How's that clearer?
> All other context menus do not (rightfully) say what they are for: mails,
> folders, accounts, toolbars. It's called a *context* menu after all.

The context menus do vary a lot, though these may be bugs.  Delete Message is the context menu for delete when opened on a message.  "Open Message in New Tab" or "Open Message in New Window" are also used.  Delete is the context menu on a folder.  When consistency is only for the sake of consistency I don't think it makes sense as a rule.  

My guideline is that we want to be specific when it doesn't create visual redundancy.  i.e. If there are 8 message commands we don't want to say " Message" after each (Archive Message, Delete Message, Move Message gets redundant and makes it hard to read).  However if there is one command or a few or they are separated by a good deal of space then I think it makes sense to use the noun as context.

Comment 6

[Magnus Melin [:mkmelin]]

By that guideline i'd still go for just the noun - http://imagebin.ca/view/INgUYm77.html - we'd have 4 commands (of 4 in the section) for the same context. That is happens to not have more commands than 4 seems irrelevant.

Comment 7

[Jae-Seong Lee-Russo]

Attachment: Add plural; Edit menu label & accesskey;

Comment 8

[Jae-Seong Lee-Russo]

Attachment: Add plural; Edit menu label & accesskey;

The old patch had an incorrect focus fix on attachmentBucket, and needs an update since Bug 503794 is fixed.

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 414188 [details] [diff] [review]
Add plural; Edit menu label & accesskey;

The wording still works for me.  Sorry for the delays!

Comment 10

[Nomis101]

This is a very usefull patch. Will this go into 3.1? I ask because string freeze is in a few days (if you can trust the schedule).

Comment 11

[Ludovic Hirlimann [:Usul]]

Maybe the review request should be directed at someone less buzy than philor ...

Comment 12

[Jae-Seong Lee-Russo]

Attachment: Add plural; Edit menu label & accesskey;

Updated so that there is no more patch hunks.

Comment 13

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

Comment on attachment 434286 [details] [diff] [review]
Add plural; Edit menu label & accesskey;

Why did you stop using PluralForm?  Based on my understanding of localization, that was the right thing to do, and this patch won't work for some of the languages we want to support.

(Also, I, too, apologize for the delay in reviews.  Apparently I'm not much less busy than philor.)

Comment 14

[Jae-Seong Lee-Russo]

Attachment: PluralForm, Edit menu label & accesskey, 5

Comment 15

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

Comment on attachment 441224 [details] [diff] [review]
PluralForm, Edit menu label & accesskey, 5

I like the change.  We need tests before this lands, but I'm happy to review those as a separate patch.

Comment 16

[Jae-Seong Lee-Russo]

Attachment: test

After rebuilding thunderbird, I am seeing this error when I run mozmill tests.

TEST-UNEXPECTED-FAIL |  setupModule
  EXCEPTION: this._treeElement is undefined
    at: folderPane.js line 345
            folderPane.js 345
       create_folder("generalContentPolicy") test-folder-display-helpers.js 327
            test-attachment.js 91
            frame.js 463
            frame.js 498
            frame.js 562
            frame.js 420
            frame.js 574
            server.js 164
            server.js 168
TEST-PASS |  test_attachment
TEST-PASS |  test_selected_attachments_are_cleared
TEST-PASS |  test_attachments_menu
TEST-UNEXPECTED-FAIL |  teardownModule
  EXCEPTION: You need to specify a tab!
    at: tabmail.xml line 400
       Error("You need to specify a tab!")  0
       _getTabContextForTabbyThing((void 0),false) tabmail.xml 400
       closeOtherTabs((void 0)) tabmail.xml 710
       teardownModule([object Object]) test-folder-display-helpers.js 295
            frame.js 463
            frame.js 542
            frame.js 562
            frame.js 420
            frame.js 574
            server.js 164
            server.js 168

Comment 17

[Jae-Seong Lee-Russo]

Comment on attachment 444968 [details] [diff] [review]
test

Let me rework on this.  I can run mozmill test fine now, and I need to redo the test.  I am sorry.

Comment 18

[Jae-Seong Lee-Russo]

Attachment: test, 1

Please review this.

Comment 19

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

Comment on attachment 445591 [details] [diff] [review]
test, 1

>+++ b/mail/test/mozmill/attachment/test-attachment.js	Sun May 16 05:10:49 2010 -0700
>@@ -185,3 +185,53 @@
>+function test_attachments_menu() {
>+  cwc.sleep(1000);

Why are you sleeping for a second in so many places?  This really slows down the tests, and I would be much happier if you could remove it.  (If you _really_ need to spin the event loop, cwc.sleep(0) will do that.)

Other than that, I like it, so r=me if you remove or fix those calls.

Thanks,
Blake.

Comment 20

[Jae-Seong Lee-Russo]

Attachment: combined

I removed cwc.sleep (I couldn't see visual changes without the sleepings), and combined two patches to one.  Do I need sr?  If not, check-in please.

Comment 21

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/07593527aa7c