Add options for Thunderbird Mac dock icon.

RESOLVED FIXED in Thunderbird 30.0

Status

Thunderbird
Preferences
--
enhancement
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Brad, Assigned: Javi Rueda)

Tracking

Thunderbird 30.0
All
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 8 obsolete attachments)

77.30 KB, image/png
bwinton
: ui-review+
Details
380.89 KB, image/png
bwinton
: review-
Details
99.19 KB, image/png
bwinton
: ui-review+
Details
92.36 KB, image/png
bwinton
: ui-review+
Details
66.21 KB, image/png
bwinton
: ui-review+
Details
14.06 KB, patch
standard8
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10
Build Identifier: 

As was discussed in 
https://bugzilla.mozilla.org/show_bug.cgi?id=518828
the old TB2 behavior on OSX was to show new mail instead of unread on the icon in the dock and this was fixed.  However, on TB3 one can only change this setting by editing config files with directions in the above bug's history.  Reading bugs here or even editing the config files in general requires a technical user, so a better way to change this behavior from unread to new in needed.

A GUI option would be optimal from a usability standpoint (in Advanced - Reading & Display?).  I personally think a running count of new mail would be great for the Windows/Linux clients, but really all the current issue requires is a GUI option setting.

Reproducible: Always

Steps to Reproduce:
1. Look for way to set icon message count from unread to new.
2. Fail at find a way in the GUI.

Actual Results:  
Gave up.

Expected Results:  
A setting for icon message count in Advanced - Reading & Display or similar.
(Reporter)

Updated

7 years ago
Version: unspecified → 3.1
Bryan thoughts on the issue ?
Severity: minor → enhancement
Component: General → Preferences
QA Contact: general → preferences
in bug 518828 I gave mockups as attachment 426407 [details] and attachment 426411 [details] for where we could put the dock options to modify those prefs.  Humph isn't working on that stuff anymore so we would need someone else to pick up the torch.
Confirming based on Bryan's comment 2.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: helpwanted
Hardware: x86 → All
(Assignee)

Comment 4

5 years ago
Created attachment 751365 [details]
Draft screenshot

A screenshot taken from a proposal to solve this "bug". The proposed images on related bug adds a new dialog box *especific* for OS X, which I am almost sure wouldn't be ok, as it would be, probably the first dialog box which would be seen only on one of the supported platforms.

Could you take a look to this one and say what do you think, Blake? Thank you.
Attachment #751365 - Flags: ui-review?(bwinton)
I prefer Bryan's mockups, if we're going to do something like this, since as soon as we open this can of worms, people will request all sorts of options for the dock icon.
Attachment #751365 - Flags: ui-review?(bwinton) → ui-review-
(Assignee)

Comment 6

5 years ago
Created attachment 753748 [details]
Draft for Dock options subdialog

Using the mockup from Bryant, then I created a new sub-dialog for setting the preferences for Dock options.

This is a draft. Maybe strings may be changed or even general UI on it.

Thoughts on it, Blake? Thank you.
Attachment #753748 - Flags: ui-review?(bwinton)
(Assignee)

Updated

5 years ago
Attachment #753748 - Attachment description: Draft por Dock options subdialog → Draft for Dock options subdialog
Comment on attachment 753748 [details]
Draft for Dock options subdialog

I can't see the button behind it, but yeah, something like that is what I'm thinking of.  Perhaps we could even use that dialog to let people choose between "new" messages and "unread" messages…
Attachment #753748 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 8

5 years ago
Created attachment 753848 [details]
New General pane

The General Pane with the button to open Dock icon options. No many differences with Byan mock-up.
Attachment #751365 - Attachment is obsolete: true
Attachment #753848 - Flags: ui-review?(bwinton)
(Assignee)

Comment 9

5 years ago
Created attachment 753851 [details]
Reworked draft for Dock options sub-dialog

Attachment 753748 [details] re-organised so it is clearer what will appear on the Dock icon's badge.

This is an alternative draft, so I am not obsoleting the other attachment.
Attachment #753851 - Flags: ui-review?(bwinton)
Comment on attachment 753848 [details]
New General pane

I'm not _totally_ happy with the phrase "Dock Options…", but ui-r=me for the general idea, and we can figure out the wording later.
Attachment #753848 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 753851 [details]
Reworked draft for Dock options sub-dialog

And this looks good, too.
Attachment #753851 - Flags: ui-review?(bwinton) → ui-review+
Summary: Thunderbird 3 Mac new email dock icon change in GUI → Add options for Thunderbird Mac dock icon.
(Assignee)

Comment 12

4 years ago
Should the dock options be opened as a modal dialog when opened from the dock icon?

Appearance is exactly the same as in attachment 753851 [details], but depending on the TB main window instead of General peferences dialog.
Assignee: nobody → leofigueres
What do you mean by "When opened from the dock icon"?
Flags: needinfo?(leofigueres)
(Assignee)

Comment 14

4 years ago
(In reply to Blake Winton (:bwinton) from comment #13)
> What do you mean by "When opened from the dock icon"?

What I understood about this bug is that a new dialog must be created in order to configure options of the Thunderbird OS X icon that appears on the Dock. There are attachments showing how this dialog looks when opened from the Preferences dialog box.

The other path to show this dialog is by right clicking on the Dock icon itself and selecting "Dock options..." -or whatever it will be phrased-. That was from attachment 426407 [details] by :clarkbw

On my comment I was trying to describe how the Dock Options dialog looks when user arrives from this last path.

Maybe I misunderstood that part of the bug :/ :\
Flags: needinfo?(leofigueres)
Ah, I didn't see that mockup (probably because it's on bug 518828 instead of this one…  ;)

So, it seems to me that having a modal preference dialog appear on the main window would be a little odd.  I would prefer to open the Preferences window and hang the modal dialog off of there.  (And I _think_ I did something similar for the BigFiles attachment handling stuff, so I believe it's technically possible.)

Thanks for enlightening me!  :)
(Assignee)

Comment 16

4 years ago
Created attachment 824560 [details] [diff] [review]
Attchmnt 753851 implementation

A new dialog has been implemented in dockoptions.xul. Locale is in dock options.dtd. There is no JavaScript needed, as Preferences service is going to manage settings automatically.

The new dialog can be accessed from 2 different locations: General pane and the application icon on the OS X Dock. Both paths will provide exactly the same user interface.

On the General pane, a new button has been added on general.xul. Code is in general.js. Some checkboxes are moved from General pane into Dock Options sub dialog.

Right clicking the Dock icon will show the label “Dock Options…”. Selecting it will open Preference window on the General pane and after that it will open Dock Options sub dialog.

Code for that last path is in macMessengerOverlay.xul and macMessengerOverlay.js. Locale for the menu item is in messenger.dtd. A modification to messenger.xul is needed so the menu is loaded when the application starts.

All new files are added to jar.mn and they will only be added to messenger.jar if platform is OS X.

From previous comments, some labels will need some re-wording.

Don't know exactly who could review this code, so I am just guessing Magnus could do it. However, it is very likely this is not going to be the patch finally lands. Also, the most likely minus-able code is in macMessengerOverlay.js which is a new file and so it hasn't been reviewed by anyone.
Attachment #824560 - Flags: ui-review?(bwinton)
Attachment #824560 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 17

4 years ago
Created attachment 824563 [details] [diff] [review]
601263.patch

Wrong patch. Sorry. This is the correct one. The other was an old backup I did.
Attachment #824560 - Attachment is obsolete: true
Attachment #824560 - Flags: ui-review?(bwinton)
Attachment #824560 - Flags: review?(mkmelin+mozilla)
Attachment #824563 - Flags: ui-review?(bwinton)
Attachment #824563 - Flags: review?(mkmelin+mozilla)

Comment 18

4 years ago
Comment on attachment 824563 [details] [diff] [review]
601263.patch

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

I don't have a mac, so i'm not a good reviewer for this. Here's some general code comments though

::: mail/base/content/macMessengerOverlay.js
@@ +9,5 @@
> +
> +// Load and add the menu item to the OS X Dock icon menu.
> +addEventListener("load",   function() {  
> +  let Ci = Components.interfaces;
> +     

remove the trailing whitespace

@@ +11,5 @@
> +addEventListener("load",   function() {  
> +  let Ci = Components.interfaces;
> +     
> +  let dockMenuElement = document.getElementById("menu_mac_dockmenu");
> +  if (dockMenuElement != null) {

we prefer to just do falsy comparisons, like if (docMenuElement)

but why would it ever not exist?
if there's a reason for it, you could also prefer to return early if that's the case

@@ +23,5 @@
> +    dockSupport.dockMenu = nativeMenu;
> +  }
> +}, false);
> +
> +function listener (event) {

no space before parenthesis, and "listener" is a too general name.

@@ +28,5 @@
> +  let preWin;
> +  setTimeout(function() {
> +    preWin = Services.wm.getMostRecentWindow("Mail:Preferences");
> +    preWin.removeEventListener("load", listener, false);
> +    preWin.document.getElementById("dockOptions").click();

... though i'm not sure what it's supposed to do. likely not doing it the right way if you have to click() on an element (call the code directly instead), and at all doing it from a timeout....

@@ +43,5 @@
> +  };
> +}
> +
> +// Show the Dock Options sub-dialog hanging from the Preferences window.
> +// This will happen even when Preferences was already opened.

Javadoc/doxygen comment style for functions please.
/**
 *
 */

::: mail/components/preferences/dockoptions.xul
@@ +1,3 @@
> +<?xml version="1.0"?>
> +
> +<!-- -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*-

remove the empty line

@@ +13,5 @@
> +<prefwindow id="DockOptionsDialog" type="child"
> +            xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +            title="&dockOptionsDialog.title;"
> +            dlgbuttons="accept,cancel"
> +            style="width: &window.macWidth; !important;">

!important doesn't look like its needed?

@@ +27,5 @@
> +                accesskey="&bounceSystemDockIcon.accesskey;"/>
> +      <groupbox flex="1">
> +        <caption label="Dock icon badge"/>
> +        <label value="On the dock icon show:"/>
> +        <radiogroup id="dockCount" preference="mail.biff.use_new_count_in_mac_dock" class="indent" 

trailing whitespace, here and a couple of other places in the file

@@ +31,5 @@
> +        <radiogroup id="dockCount" preference="mail.biff.use_new_count_in_mac_dock" class="indent" 
> +                    orient="vertical">
> +          <radio value="false" 
> +                 label="&showAllUnreadMessagesCount.label;" accesskey="showAllUnreadMessagesCount.accesskey"
> +                 id="system"/>

put ids as first attibute, and name it less generically, e.g.  dockCountSystem
Attachment #824563 - Flags: review?(mkmelin+mozilla) → review-
Comment on attachment 824563 [details] [diff] [review]
601263.patch

UI-wise, I think it's pretty reasonable.  If there were more options, we might want to use a drop-down instead of radio buttons, but for only two, radio buttons work just fine, so ui-r=me!

Thanks,
Blake.
Attachment #824563 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 20

4 years ago
Thank you Blake.

I am improving the patch with previous comment's needed changes. Furthermore, some hardcoded strings have been moved to its .dtd. I think this will not take any effect from the ui point of view.

Also thank you to Magnus for taking his time to review this patch although he has not been able to test it. I will need to find another reviewer when I fix the patch.
My pleasure!  And I apologize for taking so long to review it…
(Assignee)

Comment 22

4 years ago
Created attachment 831722 [details] [diff] [review]
Add options for Thunderbird Mac dock icon
(Assignee)

Updated

4 years ago
Attachment #824563 - Attachment is obsolete: true
(Assignee)

Comment 23

4 years ago
Comment on attachment 831722 [details] [diff] [review]
Add options for Thunderbird Mac dock icon

This patch ammends comments for code previously done by Magnus.

In order to know what is this all about, see comment 14.

I request :mbanner revew as I think he is using a Mac. Anyway, Could you take a look at this one, Mark? If not, could you route to someone who could? Thank you :)
Attachment #831722 - Flags: review?(mbanner)
Comment on attachment 831722 [details] [diff] [review]
Add options for Thunderbird Mac dock icon

I'm going to defer to Mike for now - or someone else if they've got a Mac. If they haven't got time, I may get time in a week or so to look at this.
Attachment #831722 - Flags: review?(mconley)
Attachment #831722 - Flags: review?(mbanner)
Attachment #831722 - Flags: review?
(Assignee)

Comment 25

4 years ago
Thank you, Mark.
(Assignee)

Comment 26

4 years ago
Comment on attachment 831722 [details] [diff] [review]
Add options for Thunderbird Mac dock icon

During the test to see if patch applies without problems, I have detected that a weird menu widget appears on the bottom of the main Thunderbird window. Sorry for the cancellation of the review, Mike and Mark.

The problem was that I used <menu> while I had to use <popupset>. This is now corrected on my code.

I will inspect the rest of the patch to see if there are more problems and then re-submit it again.

What about the strings which have been added, Blake? Any change needed?
Attachment #831722 - Flags: review?(mconley)
Attachment #831722 - Flags: review?
Comment on attachment 831722 [details] [diff] [review]
Add options for Thunderbird Mac dock icon

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

Comments below.

::: mail/locales/en-US/chrome/messenger/preferences/dockoptions.dtd
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!ENTITY dockOptionsDialog.title               "Dock icon options">

Apple seems to call these "app icon" or "application icon", as in the Notification Center's "Badge app icon", or the Dock Preferences' "Minimize windows into application icon" options.

I think I would be okay with picking either of those, as long as we're consistent.  :)

@@ +11,5 @@
> +<!ENTITY newMessagesCountDock.accesskey        "n">
> +<!ENTITY showAllUnreadMessagesCount.label      "Total count of unread messages">
> +<!ENTITY showAllUnreadMessagesCount.accesskey  "u">
> +<!ENTITY dockIconBadge.label                   "Dock icon badge">
> +<!ENTITY dockIconShow.label                    "On the dock icon show:">

And this would shift to "Badge app icon with:"…
(Assignee)

Comment 28

4 years ago
Created attachment 8343241 [details]
Screenshot of the App Icon Options sub-dialog box

Reworded sub-dialog. On the patch there is a title for it, but as it is a dependant dialog, title doesn't appear. The menu item added to the Dock icon is also visible.

General pane -attachment 753848 [details] hasn't been changed from previous screenshot.

Is ok the text in General pane button that opens the sub-dialog, Blake?
Attachment #753748 - Attachment is obsolete: true
Attachment #8343241 - Flags: review?(bwinton)
Comment on attachment 8343241 [details]
Screenshot of the App Icon Options sub-dialog box

No, I think we should use "app icon" everywhere, even on the General pane.

Thanks,
Blake.
Attachment #8343241 - Flags: review?(bwinton) → review-
(Assignee)

Comment 30

4 years ago
Created attachment 8343270 [details]
subdialog.png

Replaced "Dock icon" to "App icon"
Attachment #753851 - Attachment is obsolete: true
Attachment #8343270 - Flags: review?(bwinton)
(Assignee)

Comment 31

4 years ago
Created attachment 8343271 [details]
general.png

Replaced "Dock icon" to "App icon"
Attachment #8343271 - Flags: review?(bwinton)
(Assignee)

Comment 32

4 years ago
Created attachment 8343273 [details]
Captura de pantalla 5774-04-02 a la(s) 22.19.38.png

Dock menu now shows "App icon options"
Attachment #8343273 - Flags: review?(bwinton)
Comment on attachment 8343273 [details]
Captura de pantalla 5774-04-02 a la(s) 22.19.38.png

Capital "I" on "Icon" in this case, but other than that, ui-r=me.  :)
Attachment #8343273 - Flags: review?(bwinton) → ui-review+
Comment on attachment 8343270 [details]
subdialog.png

"Show count of new messages on the app icon" should be:
"Count of new messages", to match the option above.
(And let's change the option above to "Count of unread messages", for consistency.

ui-r=me with that change.
Attachment #8343270 - Flags: review?(bwinton) → ui-review+
Comment on attachment 8343271 [details]
general.png

Looks good!  ui-r=me.
Attachment #8343271 - Flags: review?(bwinton) → ui-review+
(Assignee)

Comment 36

4 years ago
Created attachment 8343904 [details] [diff] [review]
Add options for Thunderbird Mac dock icon
(Assignee)

Updated

4 years ago
Attachment #831722 - Attachment is obsolete: true
(Assignee)

Comment 37

4 years ago
Comment on attachment 8343904 [details] [diff] [review]
Add options for Thunderbird Mac dock icon

Tentatively requesting Mike Conley review. Not sure if he could try it on a Mac.

Testing it on a non-Mac platform would be interesting also, althought it could be done on the try-server.

This patch is, mostly, the same as the one it obsoletes. It just changes some entities for locales.

Could you take a look at this one, Mike? Thank you.
Attachment #8343904 - Flags: review?(mconley)
Thanks Javi - I'll get to this soon - hopefully this weekend.
(Assignee)

Comment 39

4 years ago
Any thought, Mike? :)
Flags: needinfo?(mconley)
Comment on attachment 8343904 [details] [diff] [review]
Add options for Thunderbird Mac dock icon

Tentatively redirecting to Magnus in an effort to unblock people in my review queue.
Attachment #8343904 - Flags: review?(mconley) → review?(mkmelin+mozilla)
Flags: needinfo?(mconley)

Comment 41

4 years ago
Comment on attachment 8343904 [details] [diff] [review]
Add options for Thunderbird Mac dock icon

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

Don't have a Mac so bouncing this off to Mark.

::: mail/base/content/macMessengerOverlay.js
@@ +5,5 @@
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +let Cc = Components.classes;
> +let Ci = Components.interfaces;

We don't (want to) declare these globally in thunderbird.

@@ +39,5 @@
> + * When the Preferences window is opened/closed, this observer will be called.
> + * This is done so subdialog opens as a child of it.
> + */
> +function PrefWindowObserver() {
> +  this.observe=function(aSubject, aTopic, aData) {

spaces around =

@@ +40,5 @@
> + * This is done so subdialog opens as a child of it.
> + */
> +function PrefWindowObserver() {
> +  this.observe=function(aSubject, aTopic, aData) {
> +    if (aTopic=="domwindowopened") {

spaces around ==

@@ +50,5 @@
> +}
> +
> +/**
> + * Show the Dock Options sub-dialog hanging from the Preferences window.
> + * This will be even when Preferences was already opened.

I don't understand what this is supposed to mean. Something wrong with the sentence?
Attachment #8343904 - Flags: review?(mkmelin+mozilla) → review?(mbanner)
(Assignee)

Comment 42

4 years ago
(In reply to Magnus Melin from comment #41)

> 
> @@ +50,5 @@
> > +}
> > +
> > +/**
> > + * Show the Dock Options sub-dialog hanging from the Preferences window.
> > + * This will be even when Preferences was already opened.
> 
> I don't understand what this is supposed to mean. Something wrong with the
> sentence?

"Show the Dock Options sub-dialog hanging from the Preferences window, even if Preferences window was already opened". Sorry, maybe, it was needed something like "This will be like this even when Preferences was already opened" on the original comment.

All those changes about spaces and the globally declared constants will be fixed when Mark -or whoever- reviews the patch.

Thank you for your time, Magnus. Kiitos.
Comment on attachment 8343904 [details] [diff] [review]
Add options for Thunderbird Mac dock icon

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

This looks good. I would recommend a follow-up that re-badges the doc icon when you change the preference, but I'm happy for that to be done in a different bug.

Just giving this feedback+ for now, as I'd like to see Magnus' comments addressed before giving r+.
Attachment #8343904 - Flags: review?(standard8) → feedback+
(Assignee)

Comment 44

4 years ago
Created attachment 8381663 [details] [diff] [review]
601263.patch

Patch with fixed previous commented problems.

Tested on latest trunk available right now. It builds and runs ok. I wasn't able to check it on other platforms, however.
Attachment #8381663 - Flags: review?(standard8)
Comment on attachment 8381663 [details] [diff] [review]
601263.patch

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

Ok, thanks for mentioning other platforms. One minor issue to follow-up on. r=Standard8 with that fixed.

::: mail/base/content/messenger.xul
@@ +20,4 @@
>  <?xul-overlay href="chrome://messenger/content/specialTabs.xul"?>
>  <?xul-overlay href="chrome://messenger/content/webSearchTab.xul"?>
>  <?xul-overlay href="chrome://messenger/content/quickFilterBar.xul"?>
> +<?xul-overlay href="chrome://messenger/content/macMessengerOverlay.xul"?>

Ah, now you mention about other platforms... this should be wrapped in an ifdef, i.e.

#ifdef XP_MACOSX
<?xul-overlay href="chrome://messenger/content/macMessengerOverlay.xul"?>
#endif
Attachment #8381663 - Flags: review?(standard8) → review+
(Assignee)

Comment 46

4 years ago
Fixed the ifdef for macMessengerOverlay.xul. I also found that there was a dockoptions.dtd (at the locales dir) which had not been ifdef'd. Should I?

Thank you for your review, Mark :) ^^
(In reply to Javi Rueda from comment #46)
> Fixed the ifdef for macMessengerOverlay.xul. I also found that there was a
> dockoptions.dtd (at the locales dir) which had not been ifdef'd. Should I?

No, dtd files should be left without ifdefs (l10n doesn't handle ifdefs).
(Assignee)

Comment 48

4 years ago
Created attachment 8384723 [details] [diff] [review]
Patch with ifdef'd Mac especific files

This patch fixes Mark's last comment ifdef needed about macMessengerOverlay.xul, which should be included only on OS X builds.

Also it ifdefs dockoptions.dtd on the jar.mn for locales. On comment 46 I asked about this one, but forgot to mention that I was referring to the jar.mn, no the .dtd itself.

Not requesting for checking as Mark would like to see those changes before.

Thank you Mark, Magnus, Mike and Blake for your reviews ^^
(Assignee)

Comment 49

4 years ago
Comment on attachment 8384723 [details] [diff] [review]
Patch with ifdef'd Mac especific files

Could you please confirm the r+ with this new change on locales/jar.mn, Mark? Thank you.
Attachment #8384723 - Flags: review?(standard8)
Comment on attachment 8384723 [details] [diff] [review]
Patch with ifdef'd Mac especific files

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

I'm not quite convinced we should have the ifdef in the locales/jar.mn, but it appears that Firefox does this in various places, so it doesn't seem to cause any harm. Therefore r=Standard8
Attachment #8384723 - Flags: review?(standard8) → review+
(Assignee)

Comment 51

4 years ago
Comment on attachment 8381663 [details] [diff] [review]
601263.patch

Obsoleted by newest one with ifdef on jar.mn for locales. Thank you for takeing your time in reviewing those patches, Mark
Attachment #8381663 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: helpwanted → checkin-needed
Attachment #8343904 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/f90cbb166eb6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 30.0
You need to log in before you can comment on or make changes to this bug.