887385 - Warn when user opens many selected messages in new tabs e.g. with Enter key

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, enhancement)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Thunderbird should warn if a user opens many selected messages in new tabs e.g. by pressing the Enter key while they are selected.

Comment 1

[:aceman]

This is already implemented if the preference says to open msgs in windows. Maybe the deciding condition at that place just needs to be tweaked and new string added, using the word 'tabs' instead of 'windows'. I think I was looking at that code recently but not sure where.

Comment 2

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch

Please let me know if this is the desired behaviour, I'll add the string IDs and make a common function for both tabs and windows so that duplication is removed.

Comment 3

[:aceman]

Comment on attachment 768372 [details] [diff] [review]
Patch

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

Whoa Suyash, that's it!
It has some small tweaks to do (e.g. the warning should be shown only when if(aTabmail) is true) but you found the right place in code.

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 768372 [details] [diff] [review]
Patch

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

Thank you for the fast patch.
A) Should the limit be the same for new tabs and windows? It's much cheaper from the performance side to open tabs than windows (memory + cpu).
B) In my humble opinion, new strings should be used. It's possible that some localizers used something "You are going to open %S new windows..."
At least B should be fixed by copying and modifying the entity name from the window case.

Comment 5

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Made the changes.

Comment 6

[:aceman]

Comment on attachment 768563 [details] [diff] [review]
Patch v2

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

::: mail/base/modules/MailUtils.js
@@ +139,5 @@
>     *                   - if no 3pane windows are open, standalone windows are
>     *                     opened instead of tabs
>     */
> +
> +  confirmAction: function (aMsgHdrs, confirmTitle, confirmation, warning) {

Great code reuse! Just change aMsgHdrs to aNumMessages (and pass in the count directly)
and rename the other arguments to start with 'a', like aConfirmTitle. 'warning' should probably be like 'aLimitingPref'.

Add description of the arguments. Like:
/**
 * Warn if the number of messages to be opened is larger than the pref allows.
 *
 * @param aNumMessages  number of msgs to be displayed
 * @param aConfirmTitle ...
 * @param aConfirmation ...
 * @param  ...
 */

@@ +251,5 @@
>     */
>     openMessagesInNewWindows:
>         function MailUtils_openMessagesInNewWindows(aMsgHdrs,
>                                                     aViewWrapperToClone) {
> +    if (this.confirmAction(aMsgHdrs, "openWindowWarningTitle",

I think you could send aMsgHdrs.length directly.

::: mailnews/mailnews.js
@@ +304,5 @@
>  pref("mailnews.reuse_message_window", true);
>  #endif
>  
>  pref("mailnews.open_window_warning", 10); // warn user if they attempt to open more than this many messages at once
> +pref("mailnews.open_tab_warning", 10); // warn user if they attempt to open more than this many messages at once

We need somebody to decide if the new pref is worth it. As opening tabs may be cheaper than windows, maybe we could inside the code handle the tab limit as getIntPref("mailnews.open_window_warning")*2 or something.

Comment 7

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Made all the changes suggested, except the pref for tabs, which is yet to be decided.

Comment 8

[:aceman]

Comment on attachment 768839 [details] [diff] [review]
Patch v3

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

Thanks, very good :)

::: mail/base/modules/MailUtils.js
@@ +119,5 @@
>      this.displayMessages([aMsgHdr], aViewWrapperToClone, aTabmail);
>    },
> +  /**
> +   * Display the warning if the number of messages to be displayed is greater than
> +   * the premissible limit.

"premissible" -> "limit set in preferences".

@@ +121,5 @@
> +  /**
> +   * Display the warning if the number of messages to be displayed is greater than
> +   * the premissible limit.
> +   * @param aNumMessages: number of messages to be displayed
> +   * @param aConfirmTitle: confirmation box title

title ID

@@ +122,5 @@
> +   * Display the warning if the number of messages to be displayed is greater than
> +   * the premissible limit.
> +   * @param aNumMessages: number of messages to be displayed
> +   * @param aConfirmTitle: confirmation box title
> +   * @param aConfirmMsg: confirmation message

message ID

@@ +123,5 @@
> +   * the premissible limit.
> +   * @param aNumMessages: number of messages to be displayed
> +   * @param aConfirmTitle: confirmation box title
> +   * @param aConfirmMsg: confirmation message
> +   * @param aLiitingPref: limiting number of messages to be displayed without warning

@param aLimitingPref  The name of the pref to retrieve the limit from.

@@ +159,5 @@
>     *                     front
>     *                   - if no 3pane windows are open, standalone windows are
>     *                     opened instead of tabs
>     */
> +  

Did you add some trailing spaces here?

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 768839 [details] [diff] [review]
Patch v3

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

So how do i open all selected messages in new tabs? 

Can we also make this apply to opening new windows/tabs for folders?

::: mail/base/modules/MailUtils.js
@@ +136,5 @@
> +            bundle.GetStringFromName(aConfirmMsg))
> +                    .replace("#1", aNumMessages);
> +          if (!Services.prompt.confirm(null, title, message))
> +            return true;
> +        }

the whole block is wrong aligned. (should be same alignment as "let openWarning="

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +408,5 @@
> +# for checking if the user really wants to open lots of messages in tabs.
> +openTabWarningTitle=Confirm
> +# LOCALIZATION NOTE (openTabWarningConfirmation): Semi-colon list of plural forms.
> +# #1 is the number of messages the user is attempting to open.
> +openTabWarningConfirmation=Opening #1 message may be slow.  Continue?;Opening #1 messages may be slow.  Continue?

No double spacing please

Comment 10

[Suyash Agarwal (:sshagarwal)]

(In reply to Magnus Melin from comment #9)
> So how do i open all selected messages in new tabs? 
I think, select all and press enter should work.

> > +openTabWarningConfirmation=Opening #1 message may be slow.  Continue?;Opening #1 messages may be slow.  Continue?
> 
> No double spacing please

And for the double spacing, should I remove this double space from openWindowWarningConfirmation too?

Comment 11

[Magnus Melin [:mkmelin]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #10)
> (In reply to Magnus Melin from comment #9)
> > So how do i open all selected messages in new tabs? 
> I think, select all and press enter should work.

Opens in windows for me. Can't recall if there's a pref.

> > > +openTabWarningConfirmation=Opening #1 message may be slow.  Continue?;Opening #1 messages may be slow.  Continue?
> > 
> > No double spacing please
> 
> And for the double spacing, should I remove this double space from
> openWindowWarningConfirmation too?

I'll leave it up to you. Usually old code can be left alone.

Comment 12

[Magnus Melin [:mkmelin]]

Ah i was mistaken. Indeed it opens in tabs for Enter.

Comment 13

[Suyash Agarwal (:sshagarwal)]

(In reply to Magnus Melin from comment #11)
> Opens in windows for me. Can't recall if there's a pref.

Ya, mail.openMessageBehavior

> I'll leave it up to you. Usually old code can be left alone.

If you suggest, I can make this change, I wrote that plural form introducing double spacing there :)

Comment 14

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3 (revised)

Fixed the double spacing and indentation.
And for the separate pref that is added for the tabs, is this accepted, or some factor of the window pref is to be used?

Comment 15

[:aceman]

Comment on attachment 769772 [details] [diff] [review]
Patch v3 (revised)

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

::: mail/base/modules/MailUtils.js
@@ +124,5 @@
> +   * the limit set in preferences.
> +   * @param aNumMessages: number of messages to be displayed
> +   * @param aConfirmTitle: title ID
> +   * @param aConfirmMsg: message ID
> +   * @param aLiitingPref: the name of the pref to retrieve the limit from

aLimitingPref (typo)

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 769772 [details] [diff] [review]
Patch v3 (revised)

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

::: mailnews/mailnews.js
@@ +303,5 @@
>  #ifdef MOZ_SUITE
>  pref("mailnews.reuse_message_window", true);
>  #endif
>  
>  pref("mailnews.open_window_warning", 10); // warn user if they attempt to open more than this many messages at once

this many windows

@@ +304,5 @@
>  pref("mailnews.reuse_message_window", true);
>  #endif
>  
>  pref("mailnews.open_window_warning", 10); // warn user if they attempt to open more than this many messages at once
> +pref("mailnews.open_tab_warning", 10); // warn user if they attempt to open more than this many messages at once

this many tabs

Comment 17

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

Comment on attachment 769772 [details] [diff] [review]
Patch v3 (revised)

This seems to work as designed, although since tabs are lighter-weight than windows, I think we could bump the warning number up to, say, 20.

ui-r=me with that change.

Comment 18

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Made the change suggested.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/28fadcfb3e19