Closed Bug 520214 Opened 10 years ago Closed 9 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a1

People

(Reporter: mkmelin, Assigned: lusian)

Details

Attachments

(2 files, 7 obsolete files)

Attached patch proposed fixSplinter Review
+++ 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
Attachment #404291 - Flags: ui-review?(clarkbw)
Attachment #404291 - Flags: review?(philringnalda)
Attachment #404291 - Flags: review?(philringnalda) → review-
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 :)
Target Milestone: --- → Thunderbird 3.1a1
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.
Attachment #404291 - Flags: ui-review?(clarkbw) → ui-review-
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.
Attached patch Use PluralForm, 1 (obsolete) — Splinter Review
Attachment #406150 - Flags: ui-review?(clarkbw)
Attachment #406150 - Flags: review?(philringnalda)
(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.
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.
Attachment #406150 - Attachment is obsolete: true
Attachment #410858 - Flags: review?
Attachment #406150 - Flags: ui-review?(clarkbw)
Attachment #406150 - Flags: review?(philringnalda)
Attachment #410858 - Flags: ui-review?(clarkbw)
Attachment #410858 - Flags: review?(philringnalda)
Attachment #410858 - Flags: review?
The old patch had an incorrect focus fix on attachmentBucket, and needs an update since Bug 503794 is fixed.
Attachment #410858 - Attachment is obsolete: true
Attachment #414188 - Flags: ui-review?(clarkbw)
Attachment #414188 - Flags: review?(philringnalda)
Attachment #410858 - Flags: ui-review?(clarkbw)
Attachment #410858 - Flags: review?(philringnalda)
Comment on attachment 414188 [details] [diff] [review]
Add plural; Edit menu label & accesskey;

The wording still works for me.  Sorry for the delays!
Attachment #414188 - Flags: ui-review?(clarkbw) → ui-review+
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).
Maybe the review request should be directed at someone less buzy than philor ...
Updated so that there is no more patch hunks.
Attachment #414188 - Attachment is obsolete: true
Attachment #434286 - Flags: review?(bwinton)
Attachment #414188 - Flags: review?(philringnalda)
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.)
Attachment #434286 - Flags: review?(bwinton) → review-
Attachment #434286 - Attachment is obsolete: true
Attachment #441224 - Flags: review?(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.
Attachment #441224 - Flags: review?(bwinton) → review+
Attached patch test (obsolete) — Splinter Review
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
Attachment #444968 - Flags: review?(bwinton)
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.
Attachment #444968 - Flags: review?(bwinton)
Attached patch test, 1 (obsolete) — Splinter Review
Please review this.
Attachment #444968 - Attachment is obsolete: true
Attachment #445591 - Flags: review?(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.
Attachment #445591 - Flags: review?(bwinton) → review+
Attached patch combinedSplinter Review
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.
Attachment #441224 - Attachment is obsolete: true
Attachment #445591 - Attachment is obsolete: true
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/07593527aa7c
Assignee: mkmelin+mozilla → lusian
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Thunderbird 3.1a1 → Thunderbird 3.2a1
You need to log in before you can comment on or make changes to this bug.