Closed Bug 759039 Opened 12 years ago Closed 7 years ago

Composition: Printing no subject (if draft not saved), or stale/old subject (if saved) in page header Title field

Categories

(Thunderbird :: Message Compose Window, defect)

Product:
Thunderbird
Component:
Message Compose Window
Version:
12 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 57.0

People

(Reporter: thomas8, Assigned: thomas8)

References

(Depends on 2 open bugs, Blocks 3 open bugs)

Details

(Whiteboard: [Not fixed for direct printing w/o Print Preview, see bug 1404677])

Attachments

(2 files, 7 obsolete files)

Screenshot 1: Text encoding in status bar
69.86 KB, image/jpeg
Details
Patch V.5.2: (add 1 function descr.) Polish compose (preview) window titles, text encoding info -> statusbar, fix print (preview) title header
15.05 KB, patch
jorgk-bmo
: review+
aceman
: review+
thomas8
: ui-review+
Details | Diff | Splinter Review 
Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/15.0 Thunderbird/15.0a1

STR

1 write new message, subject: "Old subject"
2 print (before autosave sets in), check Title (in page header of printout)
-> Title is empty (bug: should be "Old subject")

3 save draft (Ctrl+S)
4 print, check Title
-> Title = "Old subject" (ok)

5 change subject "New subject" (do *not* save draft after this)
6 print (before autosave sets in), check Title
-> Title = "Old subject" (bug: should be "New subject")

Actual Result
For most cases, we're getting it wrong by design.
-> see STR above

Expected Result
-> print the correct (current) subject as Title in page header at *any* time, regardless of draft status (see STR above)
Blocks: 230490
Blocks: 297702
Steffen, would you know any starting points in code for this bug?
Flags: needinfo?(steffen.wilberg)
In the compose window, the "printPreviewMenuItem" calls the "cmd_printPreview" command.
That executes DoCommandPrintPreview();
That calls PrintUtils.printPreview(PrintPreviewListener);

This is here:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/printing/content/printUtils.js#55

And the corresponding PrintPreviewListener is defined here:
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#251

On entering Print Preview, toggleAffectedChrome(true) is called, which has:

327     let subject = document.getElementById("msgSubject").value;
328     if (subject)
329       document.title = subject;
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#327

That sets the title of the msgComposeWindow. So if the subject is "test", it changes the window title from "Write: test" to "test".
That's unnecessary, and it doesn't affect the title of the content document, so the title of the print preview browser #cppBrowser is still the empty string, which gets displayed as "about:blank".

So instead of
  document.title = subject;
we could write
  document.getElementById("cppBrowser").contentDocument.title = subject;

But that is happening after the layout of document in print preview. You'd have to change orientation from landscape to portrait, or change the text size, to take effect.
Flags: needinfo?(steffen.wilberg)
This sets the title of the mail content to the mail subject, whenever the subject is changed.

But it has the side effect of actually adding a <title>your-subject</title> tag to the <head> section of HTML emails. I noticed that most of the HTML emails I receive have either no title tag or a title tag which doesn't match the mail subject. We seem to ignore the title tag and always display the mail subject. So the title tag doesn't seem to be harmful, just a waste of bandwidth.

It's probably better to fix this in a different way, since we want to make further changes anyway in bug 251953.
I meant bug 297702.
(In reply to Steffen Wilberg from comment #2, comment #3.

Thank you Steffen, that's more than helpful.
Whiteboard: [patchlove]
Inspired by Comment 3's draft patch, here's another proposal how to fix this.

* Add "Print Preview:" to title bar of composition window
* use msg subject as title in header of print preview
* if title tag of HTML message is present, use that as title for print preview header
- assuming that an explicit <title> tag in the msg exists for a reason (
- we're going to add subject when we improve print layout of drafts (existing bug)

By its own admission, Comment 3 wasn't the best way of solving this:
- setting <title> attribute in msg from subject might still break on certain email systems.

I've used Jörg's ThunderHTMLedit to check and edit Msg HTML.
I observed some irregularities in behaviour, e.g. sometimes <title> is automatically set from subject, but not always.

(In reply to Steffen Wilberg from comment #3)
> Created attachment 8354950 [details] [diff] [review]
> This sets the title of the mail content to the mail subject, whenever the
> subject is changed.
> ...
> It's probably better to fix this in a different way, since we want to make
> further changes anyway in bug 251953.
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Attachment #8901957 - Flags: review?(acelists)
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #6)

> I've used Jörg's ThunderHTMLedit to check and edit Msg HTML.
> I observed some irregularities in behaviour, e.g. sometimes <title> is
> automatically set from subject, but not always.

Another irregular behaviour: Sometimes when there's no subject, explicit <title> in msg does not get promoted to print preview header title. Looks like a timing problem when changes to HTML tab get saved.
Turns out that there's something wrong with my previous patch, looks like timing issue involving synchronous and asynchronous tasks.

This patch just adds one alert at the right spot, and everything works as expected. Without the alert, I am getting empty preview Window (no page seen). This reproduces every time for me.

Unfortunately, I have no idea what's going on.
Combined with the other unpredictabilities listed above, there's definitely something going on. Please advise...
Flags: needinfo?(jorgk)
Uff, why wouldn't the reviewer handle that? He's also the timing master ;-)
That says, you can run the event loop by doing something like:

setTimeout(function() {
  PrintUtils.printPreview();
}, 0);
Flags: needinfo?(jorgk)
Typo: That *said*, you can run the event loop by doing something like: setTimeout(function() { ... }, 0);
Attached patch patch v3 (obsolete) — — Splinter Review 
I played with this a bit because the added call to PrintUtils.printPreview(); looks like trouble for me. We can't call printPreview while we are already running printPreview.

I determined we need to set document.getElementById("content-frame").contentDocument.title to what we want to see in the header of the previewd document. And this must be done before getPrintPreviewBrowser() finishes (in toggleAffectedChrome() is too late). It seems to me setting .title of cppBrowser does nothing. I see you do that in the patch, so I'm not sure.

Does this work for you? It even supports the case with different message document <title> and message subject.
Attachment #8903853 - Flags: feedback?(bugzilla2007)
Blocks: 1396455
Comment on attachment 8903853 [details] [diff] [review]
patch v3

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

Thanks for playing with this! :)

* It's certainly good to avoid recursively calling printPreview(), although that scenario is described in code annotations, and it was basically working in my patch except for timing issues. Your patch avoids them by temporarily injecting the title into the message content document, but that's still just a workaround for bug 1396455, and we should mark it as such. It's a really nice workaround; I like it :)
* Let's not leave empty <title> tag behind if there wasn't one before. Internet rumor has it that some platforms choke on titles in email messages, and we don't want to meddle with user's message data.
* Imo we should not show the message content document title as preview window title, that's an unexpected change from Write window title.

Taking ideas from your patch, I've prepared a new patch which also streamlines ternary variable assignment which reads a bit clumsy here (as in my old patch).

::: mail/components/compose/content/MsgComposeCommands.js
@@ +186,5 @@
>  
>  var PrintPreviewListener = {
>    getPrintPreviewBrowser: function() {
> +    let browser = document.getElementById("cppBrowser");
> +    preparePrintPreviewContent();

Function name: We're really just doing this to tweak the *title* early, content should never be changed here because there's onEnter to do that, which works for almost everything except title.

@@ +250,5 @@
> +  let msgDocument = document.getElementById("content-frame").contentDocument;
> +  let msgSubject = document.getElementById("msgSubject").value.trim();
> +  msgSubject = !msgSubject ? getComposeBundle().getString("defaultSubject")
> +                           : msgSubject;
> +  let subject = !msgDocument.title ? msgSubject : msgDocument.title;

Too much of subject here :)
I digged a bit and found that short circuit evaluation paired with JavaScript's loose typing allows us to streamline these.
Also, let's not call it subject when it's the title!

@@ +272,5 @@
>    if (aHide)
>    {
>      // going into print preview mode
> +    // Set window title.
> +    let subject = document.getElementById("content-frame").contentDocument.title;

This isn't always the subject, but it should be always the subject (see above).

@@ +289,5 @@
>    {
>      // restoring normal mode (i.e., leaving print preview mode)
>      SetComposeWindowTitle();
> +    document.getElementById("content-frame")
> +            .contentDocument.title = gChromeState.messageTitle;

Unfortunately, when the original message didn't have a title tag, this will leave an empty <title> tag behind.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +132,5 @@
>  
>  ##
>  windowTitlePrefix=Write:
> +# LOCALIZATION NOTE (windowTitlePrefixPrintPreview): %S is the message subject
> +windowTitlePrefixPrintPreview=Print Preview: %S

Print preview of a received message has short brand name in window title - I think we should also include that here, for consistency and, well, branding!
Attachment #8903853 - Flags: feedback?(bugzilla2007) → feedback+
(In reply to :aceman from comment #11)
> I determined we need to set
> document.getElementById("content-frame").contentDocument.title to what we
> want to see in the header of the previewd document. And this must be done
> before getPrintPreviewBrowser() finishes (in toggleAffectedChrome() is too
> late). It seems to me setting .title of cppBrowser does nothing.

It does change the DOM, and therefore prints correctly, but it doesn't update the display of the print preview header, which is somewhat outside the document. For details, please see bug 1396455.
Per documentation, onEnter() which I used is the correct place for this, but due to the bug, it needs recursive printPreview() to update the display which isn't nice and probably caused the timing issues in my patch, so your workaround is better.
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #12)
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +186,5 @@
> >  
> >  var PrintPreviewListener = {
> >    getPrintPreviewBrowser: function() {
> > +    let browser = document.getElementById("cppBrowser");
> > +    preparePrintPreviewContent();
> 
> Function name: We're really just doing this to tweak the *title* early,
> content should never be changed here because there's onEnter to do that,
> which works for almost everything except title.

We don't know that as we do not change the document contents in onEnter or toggleAffectedChrome(). We only hide the panels/UI.
So the only change I would think needs to be done to the function name is make it something like tweakPrintPreviewContent() as the content is prepared we just change it a bit here.

> @@ +289,5 @@
> >    {
> >      // restoring normal mode (i.e., leaving print preview mode)
> >      SetComposeWindowTitle();
> > +    document.getElementById("content-frame")
> > +            .contentDocument.title = gChromeState.messageTitle;
> 
> Unfortunately, when the original message didn't have a title tag, this will
> leave an empty <title> tag behind.

And how is that specified? The .title property should know what to do and remove the title tag if it is set to empty. So if it doesn't it may be a bug.
So here's a fully revamped patch, including an improved variant of Aceman's tweak, and streamlined variable assignments exploiting short circuit evaluation and JavaScript's loose typing - makes more readable and more condensed code!

Richard, here's what this does:

* Consistently use subject for window titles (as we do in Write: ... window) (not document title)
* Consistent with "Write: ...", use prefix "Print Preview: ..."

* Consistent with print preview of received messages, use trailing brand name:
"Print Preview: Invitation for dinner - Thunderbird"
- imo that's still good practice for title bars if they are visible, to identify the application (from the general look of our draft preview, the originating application isn't very obvious; this might be a Word doc, email msg crafted in Word, a msg printed as pdf, another email application...)
- consistency with other apps
- let's be proud of our brand and show it!

* In Print Preview's header (and printouts; upper left corner), default to using subject, but if there's an explicit, hand-crafted <title> in the message HTML, let's use that.
- expose <title> in a non-obtrusive way if it exists
- allow custom titles for printouts
- even if there's <title> in the header, we'll still expose the subject in bug 297702.

* Clean up after our implementation-level tweak to inject subjects into the message document's <title>, which temporarily creates it if not present, i.e. let's not change/add <title> in user's message in any way.
Attachment #8354950 - Attachment is obsolete: true
Attachment #8901957 - Attachment is obsolete: true
Attachment #8902211 - Attachment is obsolete: true
Attachment #8903853 - Attachment is obsolete: true
Attachment #8901957 - Flags: review?(acelists)
Attachment #8904331 - Flags: ui-review?(richard.marti)
Attachment #8904331 - Flags: review?(acelists)
(In reply to :aceman from comment #14)
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #12)
> > ::: mail/components/compose/content/MsgComposeCommands.js
> > @@ +186,5 @@
> > >  
> > >  var PrintPreviewListener = {
> > >    getPrintPreviewBrowser: function() {
> > > +    let browser = document.getElementById("cppBrowser");
> > > +    preparePrintPreviewContent();
> > 
> > Function name: We're really just doing this to tweak the *title* early,
> > content should never be changed here because there's onEnter to do that,
> > which works for almost everything except title.
> 
> We don't know that as we do not change the document contents in onEnter or
> toggleAffectedChrome(). We only hide the panels/UI.

We know that both from documentation and because I tested it.
Please see bug 1396455 for details, which includes a test for real-time updating.

> So the only change I would think needs to be done to the function name is
> make it something like tweakPrintPreviewContent() as the content is prepared
> we just change it a bit here.

We're really just tweaking the title, not random content, which imo should reflect in the function name.
We're NOT tweaking the *printPreview* content, we're temporarily tweaking the title of *message document* content (to work around a bug), which then reflects in the preview. Let's not be misleading please.

> > @@ +289,5 @@
> > >    {
> > >      // restoring normal mode (i.e., leaving print preview mode)
> > >      SetComposeWindowTitle();
> > > +    document.getElementById("content-frame")
> > > +            .contentDocument.title = gChromeState.messageTitle;
> > 
> > Unfortunately, when the original message didn't have a title tag, this will
> > leave an empty <title> tag behind.
> 
> And how is that specified? The .title property should know what to do and
> remove the title tag if it is set to empty. So if it doesn't it may be a bug.

Wouldn't it be surprising if assigning an empty string to a property which represents the inner text of an element would remove the entire element? What if the element is still needed, e.g. to set another text from other code?
(title=="") <> nothing...  ;)
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #16)
> > So the only change I would think needs to be done to the function name is
> > make it something like tweakPrintPreviewContent() as the content is prepared
> > we just change it a bit here.
> 
> We're really just tweaking the title, not random content, which imo should
> reflect in the function name.
> We're NOT tweaking the *printPreview* content, we're temporarily tweaking
> the title of *message document* content (to work around a bug), which then
> reflects in the preview. Let's not be misleading please.

My function name was a superset of what it was doing (for now) so that isn't bad. The other way round could be a problem.
 
> > And how is that specified? The .title property should know what to do and
> > remove the title tag if it is set to empty. So if it doesn't it may be a bug.
> 
> Wouldn't it be surprising if assigning an empty string to a property which
> represents the inner text of an element would remove the entire element?

Some XUL properties do exactly what I say, see e.g. https://dxr.mozilla.org/comm-central/rev/37824bf5c5b08afa7e689fceb935b8f457ebd9eb/mozilla/toolkit/content/widgets/preferences.xml#257. The example is on an attribute level, not entire element, but the idea is the same.
Comment on attachment 8904331 [details] [diff] [review]
Patch V.4: (using Aceman's workaround, improved behaviour, streamlined code, annotations) Fix this.

Not tested but the new title looks good.
Attachment #8904331 - Flags: ui-review?(richard.marti) → ui-review+
This could take another iteration to also add " - Thunderbird" to the Write window title, for consistency. We might want to unify SetComposeWindowTitle() and setPPTitle().
Maybe having some review comments before next iteration would be good?
I need to know a few things before we continue here, from Jörg, Richard, and Aceman.

When user changes Options > Text Encoding to something other than the default (Unicode), we show that in the composition window title, like this:

Write: Invitation for dinner - Japanese (ISO-2022-JP)

Unfortunately I don't know much about text encodings, charsets and the like, maybe Jörg can enlighten me?
I understand that Unicode (UTF8 encoding, but that's strangely not shown in our display title?) handles most world languages nicely, while reducing the bandwidth needed to encode them. So I wonder why anybody would want to use another text encoding apart from that?
What are the consequences of setting a specific text encoding, a) while composing, and b) for the recipient? Does it mean if I set Japanese, I won't be able to enter Chinese?

Jörg/Richard: Is it important/consequential enough to be shown in the window title? Does this setting really belong there?
If not - can we just omit it, or perhaps show it in the status bar, before the 'spelling check language' status panel?

I suspect this setting, which I believe is now rarely used because of UTF-8, is the original reason why composition doesn't have the " - Thunderbird" branding, because the following title structure was considered undesirable:

Write: Invitation for dinner - Japanese (ISO-2022-JP) - Thunderbird

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #19)
> This could take another iteration to also add " - Thunderbird" to the Write
> window title, for consistency.

Imo, even if we think that special text encoding should be shown in the window title, we should still add the branding as we normally would, given that this is just an edge case and it seems that we actually WANT the title to be odd so that the exceptional text encoding stands out to be noticed. Richard?

Then, some review questions to Aceman:

Can we use this?
> Print Preview: %S - %S
As used in received message print preview, that's the traditional simple pattern where you can't change the order of the placeholders.
I wonder what happens when you omit one of them from translation?

Or we should use this instead?
> Print Preview: %1$S - %2$S
Here, each placeholder is unique, so their order can be changed.
Again, I wonder what happens when you omit one placeholder from translation?

And what is the best way to include a placeholder which isn't always there?
Maybe two strings, one for the case with optional placeholder, and one without?

> windowTitleCompose=Write: %S - %S     (placeholders for subject and brand)
> windowTitleComposeWithTextEncoding=Write: %S - %S - %S (placeholders for subject, text encoding, and brand)

> windowTitlePrintPreview=Print Preview: %S - %S

Another related question:
> We might want to unify
> SetComposeWindowTitle() and setPPTitle(...).

Perhaps like this?
if (!GetCharsetUIString())
{setComposeWindowTitle("windowTitleCompose")}
else
{setComposeWindowTItle("windowTitleComposeWithTextEncoding")}

Then for printpreview, we'd use
> setComposeWindowTitle("windowTitlePrintPreview")
^^ comment 20 (https://bugzil.la/759039#c20)
Flags: needinfo?(richard.marti)
Flags: needinfo?(jorgk)
Flags: needinfo?(acelists)
There are many encodings used around the world, and despite best efforts and lobbying to switch everything UTF-8 in bug 862292, that hasn't happened and IMHO is also not desirable.

Russians like to use KOI8-*, Japanese are very conservative and insist on plaintext in ISO-2022-JP and I don't see why I need to encode äöü in UTF-8 when I can do it in ISO-8859-1 or windows-1252.

As for composing a message: You can chose any encoding combined with any message content and if in the end you can't encode the message content with the desired encoding, you're auto-upgraded to UTF-8 which can encode anything.

The encoding is only shown in the window title if it differs from the default set in "Tools > Options, Display(!), Formatting, Advanced, Text Encoding". I have that set to windows-1252 and therefore I see UTF-8. The title is set here:

https://dxr.mozilla.org/comm-central/rev/0531d19ae371c148327c379822f2bfffe9fc3256/mail/components/compose/content/MsgComposeCommands.js#2927
https://dxr.mozilla.org/comm-central/rev/0531d19ae371c148327c379822f2bfffe9fc3256/mail/components/compose/content/MsgComposeCommands.js#3887

I think having the charset there is useful to the advanced user since they're made aware that the message will most likely be shipped in a charset which differs from their default. That said, I don't always pay attention to the title, so I won't be opposed to moving that information elsewhere.
Flags: needinfo?(jorgk)
I don't see why we should need to show the charset in the title. Although when it's needed to change the charset to show the message correctly it is not needed to be noted in the title to remember the correct charset. If needed, maybe better in the footer.
Flags: needinfo?(richard.marti)
(In reply to Richard Marti (:Paenglab) from comment #23)
> Although when it's needed to change the charset to show the message correctly it is
> not needed to be noted in the title to remember the correct charset.
Changing the charset while composing doesn't have any effect on the composition. The composition is in the internal Gecko format of UTF-16 anyway.

So I guess Richard is alluding to the charset override. If you have a message that's garbled and you press reply, the reply will be garbled. If you select a different charset for display to rectify the display, then replying will use that override charset. But nothing stops you switching to UTF-8 or some other charset once you have a garble-free compose window.

In that case also it's nice to know that the override has been honoured. BTW, it you make changes to the title, you had better check our tests since I believe they check titles when playing with charsets.
Ah yes, it's for composing. Then I see really no reason why this is needed in the title.
Jörg, can the Text encoding deviate from the default (and then reflect in the title) *without* manual user intervention like actively changing display encoding (resulting in reply-overriding) or tentatively changing send encoding from within composition?
E.g., for the default case, do we copy a non-UTF-8 text encoding from the source of original message and force that upon composition as text encoding for sending (if possible)?

I generally want things in the status bar to be interactive, because it's odd to see the (status of a) property right in front of you without being able to change it. But I don't want to confuse users with allowing/seducing them to change text encoding if they have no idea what this is about. But if advanced users have already actively changed it, as Jörg said, they'll probably appreciate and know what to do with the (interactive) status in the status bar UI.
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #26)
> Jörg, can the Text encoding deviate from the default (and then reflect in
> the title) *without* manual user intervention like actively changing display
> encoding (resulting in reply-overriding) or tentatively changing send
> encoding from within composition?
> E.g., for the default case,
when replying,
> do we copy a non-UTF-8 text encoding from the
> source of original message and force that upon composition as text encoding
> for sending (if possible)?
If you start  your composition with a default of, say, ISO-8859-1 and enter Korean, Japanese or Chinese text, or Hebrew, Russian, Thai, Hindi, etc. and press save, or wait for autosave, then the charset will be upgraded to UTF-8 and that reflects in the title.

Replies generally use the charset of the original message, and it shows in the title bar if it differs from the default, unless you ticked:
"Tools > Options, Display, Formatting, Advanced, Text Encoding: When possible, use the default text encoding in replies".

I'm not sure what you're getting at. There are many ways the system will not use the default charset, and we inform the user when that happens.
I think for most normal users, even some advanced users (like me), charset/encoding information is pretty meaningless. We just expect TB do do the right thing so that we can answer without getting garbage. I couldn't care less what the internal encoding is, as long as my recipients are able to read it. And you also explained that composition encoding as reflected in title bar isn't a guarantee that that encoding really gets sent. So it's even less useful to have that information if it's not 100% reliable.

If I hear Richard correctly, we seem to agree that this information is misplaced in the title bar, too prominent for a less relevant internal formatting. I actually vaguely remember seeing that information sometimes long ago and it was just irritating because it's meaningless to normal users.

So unless there are other objections or better alternatives, we could try to expose that information in the status bar instead, right side, but to the left of 'spell checking language' status bar pane if it's present.
Status bar is less prominent and doesn't affect your window title (for task switching etc.).
I'm just a little bit concerned wrt UX how that interacts with the 'spell checking language' panel, two similar settings next to each other, there might be a bit of potential for confusion again.
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #29)
> I couldn't  care less what the internal encoding is, as long as my recipients
> are able to read it.
That's exactly the point. Now imagine that you're Japanese and sending Japanese e-mail to someone who's receiving device can only display plain text ISO-2022-JP. In that case you want to make sure that they get that. You're living in a world where everyone can read UTF-8, but that world is not the entire world there is.

> And you also explained that composition encoding as reflected in
> title bar isn't a guarantee that that encoding really gets sent.
It is after a save. And it is if you don't do anything you normally don't do, like paste CJK text when sending as ISO-8859-1.
Now we're getting somewhere.

For consistency, this adds " - Thunderbird" to the "Write: ..." window title while removing the text encoding info (shown only for non-default encodings) as discussed. Having the text encoding info in the status bar looks a lot more professional imho. I kept 'Check spelling language' status bar panel on the very right because it's the more important one of the two and should have a stable position.

I finally found that %1$S, %2$S etc. as named placeholders works totally correctly out of the box with getFormattedString(theString, [string1, string2]), so I'm no longer sure why we hand-crafted those replacements elsewhere, maybe because of plural forms which is different again. Even using %1$S as subject (which will be lifted into the title) does not disturb the string replacement algorithm.

Plus polished code: less is more!
Attachment #8904331 - Attachment is obsolete: true
Attachment #8904331 - Flags: review?(acelists)
Attachment #8907207 - Flags: ui-review?(richard.marti)
Attachment #8907207 - Flags: review?(acelists)
Nit:
> * Set the compose window title using the given string name.
 * Set the window title for compose window flavors (Write | Print Preview).
Comment on attachment 8907207 [details] [diff] [review]
Patch V.5: (ux-consistency, code cleanup) Polish compose (preview) window titles, text encoding info -> statusbar, fix print (preview) title header

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #31)
> Created attachment 8907207 [details] [diff] [review]
> Patch V.5: (ux-consistency, code cleanup) Polish compose (preview) window
> titles, text encoding info -> statusbar, fix print (preview) title header
> 
> For consistency, this adds " - Thunderbird" to the "Write: ..." window title
> while removing the text encoding info (shown only for non-default encodings)
> as discussed. Having the text encoding info in the status bar looks a lot
> more professional imho.

Is the text encoding already in the status bar? I haven't seen it. Can you add a screenshot with it to ensure it is correctly shown?

ui-r+ in case the charset is correctly shown in the footer.
Attachment #8907207 - Flags: ui-review?(richard.marti) → ui-review+
It's already in the patch. Just like in the title where it was, it will only be shown if encoding is not the default encoding.
Ah in the compose window itself. I searched it in the print preview.
Comment on attachment 8907207 [details] [diff] [review]
Patch V.5: (ux-consistency, code cleanup) Polish compose (preview) window titles, text encoding info -> statusbar, fix print (preview) title header

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

Great feature polishing, Thomas.
All seems to work fine for me.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +247,5 @@
> +  // For title header of print preview, use message content document title
> +  // if existing, otherwise message subject. To apply the message subject,
> +  // we temporarily change the title of message content document before going
> +  // into print preview (workaround for bug 1396455).
> +  let msgDocument = document.getElementById("content-frame").contentDocument;

GetBrowser().contentDocument here.

@@ +266,1 @@
>  function toggleAffectedChrome(aHide)

Thanks for adding the documentation blocks.

@@ +296,4 @@
>      SetComposeWindowTitle();
> +    // Restore original "empty" HTML document title of the message, or remove
> +    // the temporary title tag altogether if there was none before.
> +    let msgDocument = document.getElementById("content-frame").contentDocument;

GetBrowser().contentDocument again.

@@ +3735,5 @@
>    }
>    event.stopPropagation();
>  }
>  
> +function updateLanguageInStatusBar()

Did this need moving around? Is it just for some logical order of functions?

@@ +3774,5 @@
> +  }
> +
> +  // No status display for default text encoding
> +  encodingStatusPanel.collapsed = !encodingUIString;
> +  encodingStatusPanel.label = encodingUIString || "";

Why is the '|| ""' needed? It seems to me GetCharsetUIString() always returns a string, even when just "".

@@ +3929,5 @@
>  }
>  
> +/**
> + * Set the compose window title using the given string name.
> + * 

Trailing space.

@@ +3933,5 @@
> + * 
> + * isPrintPreview  (optional) true:  Set title for 'Print Preview' window.
> + *                            false: Set title for 'Write' window.
> + */
> +function SetComposeWindowTitle(isPrintPreview) {

Please specify the default value: function SetComposeWindowTitle(isPrintPreview = false)

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +134,5 @@
> +# LOCALIZATION NOTE (windowTitleWrite):
> +# %1$S is the message subject.
> +# %2$S is the application name.
> +# Example: Write: Re: Invitation - Thunderbird
> +windowTitleWrite=Write: %1$S - %2$S

Finally a proper localization :) I did this for the print preview and you fixed it for the main Write title. Bonus points!
Attachment #8907207 - Flags: review?(acelists) → review+
Flags: needinfo?(acelists)
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [patchlove]
Thanks for the review! This patch tweaks the last nits.

(In reply to :aceman from comment #36)
> Comment on attachment 8907207 [details] [diff] [review]
> Great feature polishing, Thomas.

Thanks :)) Appreciation is volunteer's fuel :)

> > +function updateLanguageInStatusBar()
> 
> Did this need moving around? Is it just for some logical order of functions?

Yes, it's for logical order, twofold:
1) Correct order for process of language changing:
> InitLanguageMenu() -> OnShowDictionaryMenu() -> ChangeLanguage() -> updateLanguageInStatusBar
2) Group structurally similar status bar functions:
> updateLanguageInStatusBar()
> updateEncodingInStatusBar()

> @@ +3774,5 @@
> > +  // No status display for default text encoding
> > +  encodingStatusPanel.collapsed = !encodingUIString;
> > +  encodingStatusPanel.label = encodingUIString || "";
> Why is the '|| ""' needed? It seems to me GetCharsetUIString() always
> returns a string, even when just "".

Indeed, thanks. I condensed this a bit more, hope it's ok:
> encodingStatusPanel.collapsed = !(encodingStatusPanel.label = encodingUIString);

> > + * Set the compose window title using the given string name.
> > + * 
> Trailing space.

Won't happen again: I just discovered "Trim trailing and Save" Macro in Notepad++, and I've assigned that to Ctrl+S.
Can anyone imagine any situation where preserving trailing spaces is required? CSV maybe?
Oh, but they might still escape elimination for "Save All"...

> Please specify the default value: function
> SetComposeWindowTitle(isPrintPreview = false)

Nice.

> > +# Example: Write: Re: Invitation - Thunderbird
> > +windowTitleWrite=Write: %1$S - %2$S
> 
> Finally a proper localization :) I did this for the print preview and you
> fixed it for the main Write title. Bonus points!

:))

Please note that using %S is only safe when there's only one placeholder in the string. For more than one placeholder, we must use named placeholders %1$S, %2$S etc. (not "foo %S bar %S baz") because otherwise their order cannot be changed in the translation, which might result in wrong string layout.
Attachment #8907207 - Attachment is obsolete: true
Attachment #8908958 - Flags: ui-review+
Attachment #8908958 - Flags: review?(acelists)
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #37)
> Thanks :)) Appreciation is volunteer's fuel :)
I was 100% sure that I had posted a positive comment here, too, but it's not there. Working on many things at the same time, I sometimes close FF and unsaved changes get lost, this is not the first time it happens :-(

I actually tried this before the review to see how the status bar looked like. So from memory the comment was:
Nice work :-) - One question: In bug 1399370 we're allowing to use the status bar to change the spellcheck language. Wouldn't the logical consequence be to allow changing the encoding from the status bar as well? For consistency? Does it make sense to add it here, or maybe better in bug 1399370?

> Can anyone imagine any situation where preserving trailing spaces is required?
Yes, when working on TB source code. When you submit a patch changing one line, we don't want this to become a patch removing trailing spaces from 100 unrelated lines. My way to fix it is to look for | $| as a regular expression with Notepad++ before submitting the patch. You only need to address the lines with + at the front, that is, the trailing spaces you added.

Another positive comment: I'm surprised that you have increased the volume of high-quality patches you're sending, that has been very positively noted, even if at times I'm grumpy and say: "Clearly didn't check for duplicates". That BTW, was just a statement of fact, we all omit things at times (see I'm not even calling it "making a mistake").
Comment on attachment 8908958 [details] [diff] [review]
Patch V.5.1: (nitfixes) Polish compose (preview) window titles, text encoding info -> statusbar, fix print (preview) title header

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

Nice!

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +309,5 @@
>  <!ENTITY customizeFromAddress.label "Customize From Address…">
>  <!ENTITY customizeFromAddress.accesskey "A">
> +
> +<!-- Status Bar -->
> +<!ENTITY statusLanguageText.tooltip "Spell checking language">

Just noticed: Shouldn't that be: "Spellchecking language"?
(In reply to Jorg K (GMT+2) from comment #38)
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #37)
> > Thanks :)) Appreciation is volunteer's fuel :)
> I was 100% sure that I had posted a positive comment here, too, but it's not
> there. Working on many things at the same time, I sometimes close FF and
> unsaved changes get lost, this is not the first time it happens :-(

Oh, I see :)

> I actually tried this before the review to see how the status bar looked
> like. So from memory the comment was:
> Nice work :-) - One question: In bug 1399370 we're allowing to use the
> status bar to change the spellcheck language. Wouldn't the logical
> consequence be to allow changing the encoding from the status bar as well?
> For consistency? Does it make sense to add it here, or maybe better in bug
> 1399370?

Yes, I had thought of that already, but I'd like to externalize that into a new bug (if I may) to control my workload and for faster landing of what is already done. Even for the public record of improvements (especially UX improvements, given their scarcity), I think it's wise to keep separate things separate.

> > Can anyone imagine any situation where preserving trailing spaces is required?
> Yes, when working on TB source code. When you submit a patch changing one
> line, we don't want this to become a patch removing trailing spaces from 100
> unrelated lines.

That looks like a scenario for automated removal of spaces in all source files in an announced one-time mission.

> My way to fix it is to look for | $| as a regular
> expression with Notepad++ before submitting the patch.

Yes, I remember, but that's extra steps on the patch file which is subject to constant changes, so you'd have to repeat that step every time you change anything, which looks quite cumbersome to me.

> You only need to
> address the lines with + at the front, that is, the trailing spaces you
> added.

So I guess your regular expression could be updated to pick those lines only?

> Another positive comment: I'm surprised that you have increased the volume
> of high-quality patches you're sending, that has been very positively noted,
> even if at times I'm grumpy and say: "Clearly didn't check for duplicates".
> That BTW, was just a statement of fact, we all omit things at times (see I'm
> not even calling it "making a mistake").

Thanks for the compliment! :))
Now that I'm showered with praise here, I'm starting to suspect a strategy of bribing me into more work... :p

kkkk, the depths of human psychology and communication...
Text and subtext between the lines, difference between sent and received message/intention, external factors, and all those tricky things... ;) Omissions matter, and they can change the message... Try to tell your wife all the facts about your relationship, but omit the famous three words... 8)

We're good :)
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #40)
> Yes, I had thought of that already, but I'd like to externalize that into a
> new bug (if I may) to control my workload and for faster landing of what is
> already done. Even for the public record of improvements (especially UX
> improvements, given their scarcity), I think it's wise to keep separate
> things separate.
Hmm, I think it would make sense in bug 1399370 since you're right there switching on status indicator to a menu, so why not the other one? If you want to do it separately, I suggest to wait until after the branch date, 21st Sept., with that bug so we don't ship it inconsistently in TB 57 beta. I'll land this bug here once Aceman has done his round.

> That looks like a scenario for automated removal of spaces in all source
> files in an announced one-time mission.
I'm at it:
https://hg.mozilla.org/comm-central/rev/fd85506c79b1bbf409bd67c73bf4944ee4c2a07d
https://hg.mozilla.org/comm-central/rev/24067b756994557e666c435ef3c78865f6fc9f6b
but so far only in C++ code.

> So I guess your regular expression could be updated to pick those lines only?
Yes, but since I need to type it in again when Notepad++ dropped it off the list of choices, | $| is fastest.

> Now that I'm showered with praise here, I'm starting to suspect a strategy
> of bribing me into more work... :p
Oops, you uncovered the plot.

> We're good :)
We're mostly good, although sometimes egos get in the way and there are some clouds ;-)
(In reply to Jorg K (GMT+2) from comment #39)
> Comment on attachment 8908958 [details] [diff] [review]
> Patch V.5.1: (nitfixes) Polish compose (preview) window titles, text
> encoding info -> statusbar, fix print (preview) title header
> 
> Review of attachment 8908958 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice!

:))

> > +<!ENTITY statusLanguageText.tooltip "Spell checking language">
> Just noticed: Shouldn't that be: "Spellchecking language"?

Yes, "Spellcheck language", but I've fixed that in bug 1399370, which is a better place.
Here, I'm just relocating the string in the file so that it's grouped with the other statusbar tooltip which I added here.
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #42)
> Yes, "Spellcheck language", but I've fixed that in bug 1399370, which is a
> better place.
OK, reviewing this right now. Works fine.
(In reply to Jorg K (GMT+2) from comment #38)
> 
> Nice work :-) - One question: In bug 1399370 we're allowing to use the
> status bar to change the spellcheck language. Wouldn't the logical
> consequence be to allow changing the encoding from the status bar as well?
> For consistency? Does it make sense to add it here, or maybe better in bug
> 1399370?

I'm not sure we should expose this too much to not tempt the user to play too much with the charsets. If we show it only when the user already changed the charset, then okay.
We show it if it differs from the default.
Comment on attachment 8908958 [details] [diff] [review]
Patch V.5.1: (nitfixes) Polish compose (preview) window titles, text encoding info -> statusbar, fix print (preview) title header

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

Thanks, looks great now.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2954,5 @@
>    else
>      dump("Compose has not been created!\n");
>  }
>  
>  function GetCharsetUIString()

OK, one last nit, please document this function that it only returns a non-default charset and returns "" if there is no charset set, thus the composition uses the default. It is important as the label in the status bar relies on that. It could be expected that a getCharset function always returns something, whether the default value or not.
Attachment #8908958 - Flags: review?(acelists) → review+
Per comment 46, add function comment for GetCharsetUIString().
Attachment #8908958 - Attachment is obsolete: true
Attachment #8909028 - Flags: ui-review+
Attachment #8909028 - Flags: review?(acelists)
Comment on attachment 8909028 [details] [diff] [review]
Patch V.5.2: (add 1 function descr.) Polish compose (preview) window titles, text encoding info -> statusbar, fix print (preview) title header

Let's take this. I was already going to invent my own.
Attachment #8909028 - Flags: review?(acelists) → review+
Comment on attachment 8909028 [details] [diff] [review]
Patch V.5.2: (add 1 function descr.) Polish compose (preview) window titles, text encoding info -> statusbar, fix print (preview) title header

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

Great, thanks.
Attachment #8909028 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e16fc2c7ad68
Correct title header of composition's Print (Preview). Polish Write/Print Preview window titles, introduce text encoding in status bar. r=aceman, ui-r=Paenglab
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I took the liberty to make the comment even greater:
https://hg.mozilla.org/comm-central/rev/e16fc2c7ad68#l1.145
Target Milestone: --- → Thunderbird 57.0
(In reply to Jorg K (GMT+2) from comment #51)
> I took the liberty to make the comment even greater:
> https://hg.mozilla.org/comm-central/rev/e16fc2c7ad68#l1.145

Great! Thanks for fast landing. Nice teamwork everyone. :)
Depends on: 1404677
Depends on: 1404691
Whiteboard: [Not fixed for direct printing w/o Print Preview, see bug 1404677]
Blocks: 1417788
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: