286283 - Detaching attachments: Need to fix titles of save/select folder dialogues (correct/improve/clarify captions)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[sairuh (rarely reading bugmail)]

David, thanks a bunch for implementing bug 2920! I found a trivial issue when
trying to detach attachments. tested with 2005031506-trunk tbird bits on linux fc3.

1. in tbird, go view a message which has attachments.

2. bring up the context menu for any of the attachments in the attachment pane.

3. select either Detach or Detach All...

results: as expected, the file picker dialog appears. however, the titlebar says
"Save All Attachments"

shouldn't the titlebar say something like "Detach File(s)"?

Comment 1

[mozilla.org]

Yes, that would make more sense, though you are actually saving, then deleting.

Comment 2

[Christopher Schultz]

Technically, you are deleting the attachment from the message when you "detach".
See bug #580247 and (I guess a dupe) bug #580243.

This seems like an easy fix to a localization file somewhere. I'd be happy to fix it myself if I knew where to look.

Comment 4

[Thomas D. (:thomas8)]

Attachment: Request UI-review for better 'save dialogue' title strings when detaching attachments

Blake, I'd think this is straightforward, 3 mins to ui-review, thanks!

STR

From attachment panel of message reader,
1) select single attachment, then detach
2) select multiple, but not all attachments, then detach
3) select all attachments, then detach/detach all
and look at the titles of the upcoming save/select folder dialogues

Actual result

1) Single: Title of save dialogue is "Save Attachment" (but we are detaching, which is not the same action as just saving -> ux-error-prevention?)

2) Multiple: Title of select folder dialogue is "Select Folder", with a subcaption: "Save all attachments" (wrong, we are /detaching/, and only  /selected/ attachments)

3) All: Title of select folder dialogue is "Select Folder", with a subcaption:
"Save all attachments" (but we are /detaching/)

Expected

1) Single: Title of save dialogue should be
"Detach Attachment"

2) Multiple: If technically possible: change title bar caption of select folder dialogue to "Detach attachments: Select Folder" (sic, without 'selected' as that would make it too clumsy)
otherwise, subcaption should be: "Detach selected attachments"

3) All: If technically possible: change title bar caption of select folder dialogue to "Detach all attachments: Select Folder"
otherwise, subcaption should be: "Detach all attachments"

Reasons:
- The action of detaching is different from just saving, and user should have clear feedback on the first dialogue (ux-feedback, ux-consistency, ux-error-prevention)
- need to fix /wrong/ strings (selected vs. all attachments)

Comment 5

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

Comment on attachment 556366 [details]
Request UI-review for better 'save dialogue' title strings when detaching attachments

>If technically possible, we should change the title of the dialogue instead:

It appears that it's not technically possible (or at least, not easy).

But regardless, this seems reasonable to me, so ui-r=me.

Comment 6

[Dwayne Edwards]

Is this still an issue if so I would like to work on it.

Comment 7

[Thomas D. (:thomas8)]

(In reply to Dwayne Edwards from comment #6)
> Is this still an issue if so I would like to work on it.

Yes, this is still an issue, so you are most welcome to work on it...

Comment 8

[Dwayne Edwards]

I have completed the code however was unable to set the message "Detach selected attachments" as it seems as if the same method is called to delete both all and selected attachments. How would I go about adding this message? Please see attachments for modifications made.

Comment 9

[Dwayne Edwards]

Attachment: Messager properties sets text on dialog window

Added strings: 
DetachAttachment=Detach Attachment
DetachAllAttachments=Detach All Attachments

to file

Comment 10

[Dwayne Edwards]

Attachment: Handles detaching files

Added code:
  if(detaching)
     GetString(NS_LITERAL_STRING("DetachAllAttachments"), saveAttachmentStr);
  else
     GetString(NS_LITERAL_STRING("SaveAllAttachments"), saveAttachmentStr);
to lines 912 - 915 and 
  if(detaching)
    GetString(NS_LITERAL_STRING("DetachAttachment"), saveAttachmentStr);
  else
    GetString(NS_LITERAL_STRING("SaveAttachment"), saveAttachmentStr);
to line 817-820

Comment 11

[Thomas D. (:thomas8)]

Dwayne, thanks for working on this! :)

Looks like you've found the right starting spots to tweak this.

1) Files touched
Can you please share the DXR links of the files which you are working on?
https://dxr.mozilla.org/mozilla-central/source/
There, you can search for your files, like this: "file:abCommon.js" (without quotes).

2) Testing
Did you test your changes? If yes, how?

3) Patch file
What we ultimately need to review and implement this is a patch file (yourchanges.patch) which has only the diff(erences) between the original files and your files. Our source code is managed in Mercurial repository (Hg).

On Windows, for trivial changes, you might chose the route of least resistance and handcraft the file:
- download original source file from https://dxr.mozilla.org/mozilla-central/source/
- modify a copy of that file in your same local folder
- use WinMerge application to create the patch file (diff): Select both files in your local folder, right-click, WinMerge, then within Winmerge: Tools > Generate Patch > Style:Unified; context:7; Result: yourfolder\yourdiff.patch.

I added a trivial example on bug 11383:
attachment 8838420 [details] (original source file)
attachment 8838420 [details] (modified source file)
attachment 8838419 [details] [diff] [review] (your local diff/patch file)

Here's a real world example (using correct patch header, not your local folder paths):
Attachment 8819569 [details] [diff] (Hg patch file)

Better ways of creating functional patches:

* On Windows, install TortoiseHg application and download TB source code repository to your own computer. Then you can edit source files locally and creating patches becomes a lot easier.
However, you can't test changes which require building Thunderbird from source code.

* You're touching c++ files; hence, if you want to test your changes yourself, you have to build Thunderbird (non-trivial):
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Thunderbird_build

Comment 12

[Thomas D. (:thomas8)]

Source files and patch files must use Unix line breaks (LF), and UTF-8.
Notepad++ is an excellent source code editor for Windows.

Comment 13

[Thomas D. (:thomas8)]

(In reply to Dwayne Edwards from comment #8)
> I have completed the code however was unable to set the message "Detach
> selected attachments" as it seems as if the same method is called to delete
> both all and selected attachments. How would I go about adding this message?
> Please see attachments for modifications made.

It'll be much easier to understand your modifications made when you offer us the links to files touched and a proper patch file.
If both scenarios use the same function, and that function calls the dialogue, I'd think that you could add a trailing boolean argument to the function to indicate whether it's about all or selected attachments. Or that check could be done inside the function, before calling the dialogue. It's a bit hard to tell without knowing the actual function you're talking about. For knowing if it's all or selected attachments, I imagine there's something like selection.count which can be compared to the total number of attachments (not sure how to get that, maybe the total count of attachment tree items).

You'll probably also want to use Thunderbird Daily and install DOM Inspector Addon for Thunderbird.

Comment 14

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #11)

> I added a trivial example on bug 11383:
> attachment 8838420 [details] (original source file)
> attachment 8838420 [details] (modified source file)
Typo: attachment 8838421 [details] (modified source file)
> attachment 8838419 [details] [diff] [review] (your local diff/patch file)

Comment 15

[Dwayne Edwards]

Attachment: Changes to messenger.properties file

Thanks for the support. I have provided the DXR links to the files I have modified and attached the patch. As it relates to testing I compiled thunderbird and used an email that contained multiple attachments and manually went through the process of detaching the attachments. Still unable to determine how to differentiate between selected and all since the only section of code I was able to observe that contained the total number of attachment is a javascript file that calls the functions in nsMessenger.cpp

DXR link for modified cpp file: 
 https://dxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessenger.cpp

DXR link for modified property file:
 https://dxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.properties

Comment 16

[Dwayne Edwards]

Attachment: nsMessenger.cpp modifications

Added conditional statement to nsMessager.cpp

Comment 17

[Dwayne Edwards]

Attachment: messenger.properties changes

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 8838906 [details] [diff] [review]
nsMessenger.cpp modifications

Review of attachment 8838906 [details] [diff] [review]:
-----------------------------------------------------------------

Could you just export the hg diff for all the modifications to all files?

::: C:/Users/Dwayne/Desktop/nsMessenger.cpp
@@ +814,5 @@
>    nsString defaultDisplayString;
>    ConvertAndSanitizeFileName(aDisplayName, defaultDisplayString);
>  
> +  if(detaching)
> +	 GetString(NS_LITERAL_STRING("DetachAttachment"), saveAttachmentStr);

nit: spaces like this, and use two space indenting and braces

if (detaching) {
  GetString(NS_LITERAL_STRING("DetachAttachment"), saveAttachmentStr);
}
else {
  GetString(NS_LITERAL_STRING("SaveAttachment"), saveAttachmentStr);
}

Comment 19

[Dwayne Edwards]

Attachment: Single patch file with all changes

I apologize for that I am not very familiar with hg. I have applied the changes to the nsMessenger.cpp. Please let me know if there is any issue with this since I am still not sure if I exported the patch correctly

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 8838934 [details] [diff] [review]
Single patch file with all changes

Review of attachment 8838934 [details] [diff] [review]:
-----------------------------------------------------------------

Much better thanks! 
Please also add a commit message to your patch. For this bug that might be
"Bug 286283 - fix titles of for detach attachment dialogs. "

After that the next step is getting your patch reviewed, you can have me review it. http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree#Getting_the_patch_reviewed

To avoid whitespace, add this line to your .hgrc file's [hooks] section
post-qrefresh.whitespace = hg qdiff | (! egrep --color -C 2 -n '^\+.*\s$')

::: mailnews/base/src/nsMessenger.cpp
@@ +814,4 @@
>    nsString defaultDisplayString;
>    ConvertAndSanitizeFileName(aDisplayName, defaultDisplayString);
>  
> +  if (detaching)  {

nit: here, and elsewhere, just one space before the braces

@@ +911,4 @@
>    nsString saveAttachmentStr;
>  
>    NS_ENSURE_SUCCESS(rv, rv);
> +  

nit: some trailing whitespace snuck in here

@@ +917,5 @@
> +  }
> +  else  {
> +    GetString(NS_LITERAL_STRING("SaveAllAttachments"), saveAttachmentStr);
> +  }
> +  

whitespace

Comment 21

[Dwayne Edwards]

Attachment: Modifications done to patch to remove whitespaces

Added commit comment and removed whitespaces

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 286283.patch

Looks like Magnus is busy with other reviews, so I'll do this review for him.

I fixed the commit message and remove some trailing white-space you still had.

I'll land the patch tomorrow.

Thanks for your contribution!

Comment 23

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/eb90c44e468e1be1a2f0078e89d004244da67298

Comment 24

[Frank-Rainer Grahl (:frg)]

Are the strings not needed in suite\locales\en-US\chrome\mailnews\messenger.properties too?

Comment 25

[Jorg K (CEST = GMT+2)]

I guess so ;-(
Can I r=frg for the two line change?

Comment 26

[Frank-Rainer Grahl (:frg)]

Yes or I upload a patch and you give it an r+.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 286283-suite.patch

So sorry about missing that :-(

Comment 28

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8841524 [details] [diff] [review]
286283-suite.patch

no problem.

Comment 29

[Jorg K (CEST = GMT+2)]

Done:
https://hg.mozilla.org/comm-central/rev/23d4272c269c1df846f17300b7a8a3c998c6c88b
Thanks for being alert!

Comment 30

[Thomas D. (:thomas8)]

Thanks Jörg for speeding this up and landing. It's a nice incremental improvement. However, this doesn't fully fix this bug per STR in comment 4: When detaching only *selected* attachments (say 3 out of 4, via attachmet context menu), we still wrongly claim to "Detach *all* attachments". Assignee asked for assistance on this problem several times, lastly in comment 15. I suggest that bugs should only be marked fixed after double-checking with the specifications of expected results as developed on the bug and ui-reviewed. Otherwise, in case of (deliberate/accepted/temporary) omissions pls comment why and/or move them to a new bug. In this case, I suggest to just fix this here, for which Jörg's assistance would be most welcome.

Comment 31

[Thomas D. (:thomas8)]

Also, I haven't seen any comment why this patch doesn't realize the ui-reviewed dialogue title strings of comment 4, e.g. "Detach attachments: Select Folder". Variations might be possible as to how we distribute the necessary information between dialogue title and sub-caption, but they should be explained and undergo ui-review.

Comment 32

[Jorg K (CEST = GMT+2)]

Thanks, I see this now. Looks like we have nsMessenger::SaveOneAttachment() and nsMessenger::SaveAllAttachments(). I haven't looked at how "more than one but not all" is distinguished from "all" (which could also be just one). If someone wanted to investigate, I'm sure we'd accept another patch. The easiest way out would of course be to lose the "All" in the title, so just make it "Save Attachments". Re. comment #15: I don't see either function called in JS code (not that I looked very thoroughly).