611458 - Add Ability to Print Summary of Collapsed Threads in Message Pane

Status: REOPENED

(Thunderbird :: Message Reader UI, enhancement)

Description

[Randy Goddard]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.7 (KHTML, like Gecko) Ubuntu/10.10 Chromium/7.0.517.44 Chrome/7.0.517.44 Safari/534.7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.12) Gecko/20101027 Lightning/1.0b2 Thunderbird/3.1.6

Could we please add the ability to print the summary of collapsed threads displayed in the Message Pane?  Rather than printing each individual message, this summary screen is perfect--this would make printing of a thread sooooo much easier.  Please see screenshot.

Reproducible: Always

Steps to Reproduce:
1.Find a threaded message.
2.Collapse the thread (\) and click on the thread Subject.
3.View the thread summary in the message pane and desire to be able to print that summary.
Actual Results:  
No cut-and-paste.  No printy printy.  Just archive or delete.  :'-(

Expected Results:  
Glorious printing of the summarized collapsed thread.

Comment 1

[Randy Goddard]

Attachment: Screenshot

Comment 2

[Jonathan Protzenko [:protz]]

Attachment: Fixes the issue

This is not completely innocent, as this fixes <https://github.com/protz/GMail-Conversation-View/issues/271>. This patch does two things:
- allow the user to (easily) print a multi-message summary,
- disable print preview when there is a multi-message summary and only one message selected (the assumption "one message selected => no summary" is broken by thunderbird conversations).

Implementing print preview for the multi message summary would be an order of magnitude harder. We have our own print preview logic for messages, which is based on printEngine.xul and utilityOverlay.js; printEngine.xul takes a list of message URIs when opened. We cannot re-use this for the multimessage, since it's pure HTML. The "right" way to do this would be to:
- create a XUL window for print-previewing the multimessage,
- using the multimessage's docShell.printPreview property, to get a nsIWebBrowserPrint
- use that nsIWebBrowserPrint to print-preview the multimessage into that newly opened window,
- implement UI for the print preview window, that interacts with the nsIWebBrowserPrint,
- etc.

I don't think that's worth it :-).

Comment 3

[Jonathan Protzenko [:protz]]

I'd love to see this in 6 => requesting tracking+.

Comment 4

[Jim Porter (:squib)]

Comment on attachment 540394 [details] [diff] [review]
Fixes the issue

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

I noticed when printing the message summary that there's no styling and that the buttons get printed too. I think it'd be nice to try to get the styling to work; at the very least, I think we should try to hide the buttons, since they don't make much sense on a printout. I'm minusing this for now, since I'd like to see the printout look a bit better (and fix the comments below), but if that turns out to be really complicated, I think we can live with it the way it is.

::: mail/base/content/mail3PaneWindowCommands.js
@@ +359,5 @@
>  
> +          // Allow the user to print a thread summary. It's simple, because it's
> +          // just html, and we can always print it.
> +          if (command == "cmd_print" && !gMessageDisplay.singleMessageDisplay)
> +            return true;

It seems like the handling for cmd_print is starting to deviate from the rest of the code here. Maybe we should just pull it out into a separate block entirely. In the future, when Mail Summaries is a part of Thunderbird, I think we'll want to support printing that too, which means this will deviate even further.

@@ +386,3 @@
>            return gFolderDisplay.getCommandStatus(nsMsgViewCommandType.cmdRequiringMsgBody);
> +        else
> +          return false;

Is it possible to allow print preview just by delegating that to the multimessage window, like with printing? If not, no worries...

Comment 5

[Jim Porter (:squib)]

One other thing. This is immediately below one of your diffs:

  // Check if we have a collapsed thread selected and are summarizing it.
  // If so, selectedIndices.length won't match numSelected. Also check
  // that we're not displaying a message, which handles the case
  // where we failed to summarize the selection and fell back to
  // displaying a message.
  if (gFolderDisplay.selectedIndices.length != numSelected &&
      command != "cmd_applyFiltersToSelection" &&
      gDBView && gDBView.currentlyDisplayedMessage == nsMsgViewIndex_None)
    return false;

Should the if statement here be changed to this?

  if (!gMessageDisplay.singleMessageDisplay &&
      command != cmd_applyFiltersToSelection")

It seems like that would be equivalent (and a whole lot clearer), based on the comment.

Comment 6

[Jonathan Protzenko [:protz]]

Attachment: Updated patch

This fixes all previously mentioned comments, and adds a multimessageview_print.css file in mail/base/content. Normally, make should pick up the file, since I also modified jar.mn, but I couldn't get it to work, and I didn't feel like triggering a full rebuild.

Comment 7

[Jim Porter (:squib)]

Comment on attachment 543256 [details] [diff] [review]
Updated patch

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

This looks good, aside from the minor issues I have below. It might also be worth having someone take a look at the print layout, but we could file a followup bug to improve the styling if there are any ideas for improvements there.

::: mail/base/content/mail3PaneWindowCommands.js
@@ +362,2 @@
>            // displaying a message.
> +          if (!gFolderDisplay.singleMessageDisplay && command != "cmd_applyFiltersToSelection")

This should be gMessageDisplay.

::: mail/base/content/multimessageview.xhtml
@@ +58,4 @@
>                href="chrome://messenger/skin/multimessageview.css"/>
> +        <link rel="stylesheet" media="print" type="text/css"
> +              href="chrome://messenger/skin/multimessageview_print.css"/>
> +        <link rel="stylesheet" media="screen, print" type="text/css"

This should be chrome://messenger/content/multimessage_view.css. I think this is why you were having issues with your jar.mn file.

::: mail/base/content/multimessageview_print.css
@@ +10,5 @@
> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> + * for the specific language governing rights and limitations under the
> + * License.
> + *
> + * The Original Code is Mail utility functions for GMail Conversation View

Is this intentional? I don't have a problem with it if it is, but I just wanted to make sure you meant to reference Conversations. :)

@@ +14,5 @@
> + * The Original Code is Mail utility functions for GMail Conversation View
> + *
> + * The Initial Developer of the Original Code is
> + * Jonathan Protzenko
> + * Portions created by the Initial Developer are Copyright (C) 2010

Should be "(C) 2011".

@@ +39,5 @@
> +}
> +
> +.heading {
> +  font-size: large;
> +  font-variant: small-caps;

I'm a little worried about using small-caps, since that may be strange for some languages. Maybe it's ok, but I know that using text-transform: lowercase; is bad for German.

Comment 8

[Jim Porter (:squib)]

(In reply to comment #7)
> This should be chrome://messenger/content/multimessage_view.css. I think
> this is why you were having issues with your jar.mn file.

Whoops! I meant "chrome://messenger/content/multimessageview_print.css".

Comment 9

[Jonathan Protzenko [:protz]]

Attachment: Updated version

This addresses all issues mentioned above. However, I'm not sure I did the right thing with jar.mn, esp. I'm wondering why multimessage.css is listed in content/. For consistency, I did it too with multimessage_print.css, but I don't if that's relevant.

Comment 10

[Jonathan Protzenko [:protz]]

Attachment: Leave the css in content

As per the discussion on IRC, this leaves multimessageview_print.css bundled in content only.

Comment 11

[Jonathan Protzenko [:protz]]

http://hg.mozilla.org/comm-central/rev/1f38992f40af

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 543931 [details] [diff] [review]
Leave the css in content

Ok, I don't think this has any adverse extension implications r=me aurora.

Comment 13

[Jonathan Protzenko [:protz]]

http://hg.mozilla.org/releases/comm-aurora/rev/21ab5eff17d9

Comment 14

[Mark Banner (:standard8)]

I had to back out some of this due to unit test failures - backed up by when you do multiple select of messages, you then can't forward inline, reply or do other things.

http://hg.mozilla.org/comm-central/rev/72f40b7e9357 (this got merged onto comm-aurora as well)
http://hg.mozilla.org/releases/comm-beta/rev/b8ad48538afb

Comment 15

[Jonathan Protzenko [:protz]]

Thanks. Squib and I were trying to simplify the test above, and after discussing on IRC, it looked like it was equivalent to what squib suggested, so we went on, but obviously it was wrong. I think we can forget about that chunk, and leave the bug as fixed. squib, thoughts?

Comment 16

[Jim Porter (:squib)]

That's what I was thinking too. Apparently that code is a bit more complicated than it appears initially.

Comment 17

[Mark Banner (:standard8)]

Ok, let's leave that code without that hunk for now, and if we can always tidy it up in a follow-up bug.

Comment 18

[Wayne Mery (:wsmwk)]

using trunk build, mail.operate_on_msgs_in_collapsed_threads = true results in summary printing when selecting multiple messages from same or different thread, when thread is not collapsed (and the root message was not selected).  

Is that a result of this fix (and by design)?  Or did that behavior already exist prior to the patch?

Comment 19

[Jonathan Protzenko [:protz]]

I think this is pretty much in the spirit of the patch, that is, print the multimessagesummary or the threadsummary when they're displayed.

Comment 20

[Wayne Mery (:wsmwk)]

in http://getsatisfaction.com/mozilla_messaging/topics/printing_multiple_messages#reply_6430286 I've posted a question related to this change  (have not yet created a regression bug report).

Comment 21

[Tom White]

This is broken behavior. Users are no longer able to print emails. How do I disable this?

Comment 22

[Mark Banner (:standard8)]

Unfortunately we've just backed out the javascript parts in bug 680446. If we want to do this we'll need to find an alternative UI point of view to do this.

Comment 23

[Jim Porter (:squib)]

Would adding UI to the mail.operate_on_msgs_in_collapsed_threads pref help out here? Based on GetSatisfaction, it seems like people who want to print multiple messages at once don't care about the multimessage summary, so they could just disable that and get what they want.

Comment 24

[Mark Banner (:standard8)]

I don't think that would be fair - i.e. if I didn't use conversations extensions I would still want the multimessage summary, but may still want also to print out multiple messages at once.

Comment 25

[Jonathan Protzenko [:protz]]

Yeah I think I just need to override the cmd_print in Conversations. I don't know if there's an easy mechanism for overriding commands of I need to go monkey-patching all around. We'll see :)

Comment 26

[Mark Banner (:standard8)]

You could always add extra cmd_print commands (of a different name) and call those from the menus.

Comment 27

[Jim Porter (:squib)]

(In reply to Mark Banner (:standard8) from comment #24)
> I don't think that would be fair - i.e. if I didn't use conversations
> extensions I would still want the multimessage summary, but may still want
> also to print out multiple messages at once.

Hm, are we able to add stuff to the print dialog? Maybe we could add an option in there somehow...

Comment 28

[Mark Banner (:standard8)]

NAFAIK, e.g. mac uses the native dialog.

Comment 29

[Wayne Mery (:wsmwk)]

(In reply to Mark Banner (:standard8) from comment #22)
> Unfortunately we've just backed out the javascript parts in bug 680446. If
> we want to do this we'll need to find an alternative UI point of view to do
> this.

specifically, bug 680446 comment 10