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

RESOLVED FIXED in Thunderbird 25.0

Status

--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: aryx, Assigned: sshagarwal)

Tracking

Trunk
Thunderbird 25.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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

6 years ago
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.
(Assignee)

Comment 2

6 years ago
Created attachment 768372 [details] [diff] [review]
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.
Attachment #768372 - Flags: feedback?(archaeopteryx)
Attachment #768372 - Flags: feedback?(acelists)

Comment 3

6 years ago
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.
Attachment #768372 - Flags: feedback?(acelists) → feedback+
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.
Attachment #768372 - Flags: feedback?(archaeopteryx)
(Assignee)

Comment 5

6 years ago
Created attachment 768563 [details] [diff] [review]
Patch v2

Made the changes.
Assignee: nobody → syshagarwal
Attachment #768372 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #768563 - Flags: feedback?(acelists)

Comment 6

6 years ago
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.
Attachment #768563 - Flags: ui-review?(bwinton)
Attachment #768563 - Flags: feedback?(mkmelin+mozilla)
Attachment #768563 - Flags: feedback?(acelists)
Attachment #768563 - Flags: feedback+
(Assignee)

Comment 7

6 years ago
Created attachment 768839 [details] [diff] [review]
Patch v3

Made all the changes suggested, except the pref for tabs, which is yet to be decided.
Attachment #768563 - Attachment is obsolete: true
Attachment #768563 - Flags: ui-review?(bwinton)
Attachment #768563 - Flags: feedback?(mkmelin+mozilla)
Attachment #768839 - Flags: ui-review?(bwinton)
Attachment #768839 - Flags: feedback?(acelists)
(Assignee)

Updated

6 years ago
Attachment #768839 - Flags: feedback?(mkmelin+mozilla)

Comment 8

6 years ago
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?
Attachment #768839 - Flags: feedback?(acelists) → feedback+
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
Attachment #768839 - Flags: feedback?(mkmelin+mozilla)
(Assignee)

Comment 10

5 years ago
(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?
(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.
Ah i was mistaken. Indeed it opens in tabs for Enter.
(Assignee)

Comment 13

5 years ago
(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 :)
(Assignee)

Comment 14

5 years ago
Created attachment 769772 [details] [diff] [review]
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?
Attachment #768839 - Attachment is obsolete: true
Attachment #768839 - Flags: ui-review?(bwinton)
Attachment #769772 - Flags: ui-review?(bwinton)
Attachment #769772 - Flags: feedback?(mkmelin+mozilla)
Attachment #769772 - Flags: feedback?(acelists)

Comment 15

5 years ago
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)
Attachment #769772 - Flags: feedback?(acelists) → feedback+
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
Attachment #769772 - Flags: feedback?(mkmelin+mozilla) → feedback+
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.
Attachment #769772 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 18

5 years ago
Created attachment 775104 [details] [diff] [review]
Patch v3

Made the change suggested.
Attachment #769772 - Attachment is obsolete: true
Attachment #775104 - Flags: ui-review+
Attachment #775104 - Flags: review?(mkmelin+mozilla)
Attachment #775104 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/28fadcfb3e19
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
You need to log in before you can comment on or make changes to this bug.