Last Comment Bug 505056 - Port bug 314124 - Folder Pane Popup over folders with unseen messages
: Port bug 314124 - Folder Pane Popup over folders with unseen messages
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- enhancement with 2 votes (vote)
: seamonkey2.13
Assigned To: Philip Chee
:
Mentors:
Depends on: 496429 174234 314124
Blocks: 404580
  Show dependency treegraph
 
Reported: 2009-07-18 16:28 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2012-07-07 03:10 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 Proposed fix. (9.26 KB, patch)
2012-06-24 07:27 PDT, Philip Chee
stefanh: ui‑review+
jh: feedback+
Details | Diff | Review
Screenshot of folder name as tooltip + summaries. (6.41 KB, image/png)
2012-07-05 09:12 PDT, Philip Chee
no flags Details
Patch v1.1 back to using a label but without .tooltip-label class (8.85 KB, patch)
2012-07-05 10:05 PDT, Philip Chee
no flags Details | Diff | Review
Patch v1.2 Fix Nits. (7.23 KB, patch)
2012-07-06 03:42 PDT, Philip Chee
neil: review+
mnyromyr: superreview+
Details | Diff | Review

Description Jens Hatlak (:InvisibleSmiley) 2009-07-18 16:28:27 PDT
TB bug 314124 implemented a tooltip showing information like sender, subject and preview for up to eight NEW messages of a folder (e.g. IMAP Inbox, RSS feed folder). SM already has the corresponding code in its mailWidgets.xml and bug 174234 will enable the tooltip (initially only to show full newsgroup names). What's left to do is porting the three mail.biff.alert.* prefs (and decide whether to activate them) and adding the styling through (theme-specific) CSS.

Note: mind bug 496429 for the prefs.
Comment 1 Michael Lueck 2010-11-30 09:44:56 PST
I understand that this bug is responsible for the request I am making...

-------- Original Message --------
Subject: Adjusting new email notification to give a teaser of the new message
Date: Tue, 30 Nov 2010 09:40:18 -0500
Newsgroups: mozilla.dev.apps.seamonkey

Greetings,

Again missing TB 2.x functionality. That code would show the message subject and teaser of arriving emails.

I have over 700 folders, many of which have unread mail in them.

Giving such information in the pop-up new message indicator would expedite if I need to attend to the new message immediately or not.

So, could that code get borrowed from TB 2.x please? Thanks!
Comment 2 Philip Chee 2012-06-24 07:27:59 PDT
Created attachment 636161 [details] [diff] [review]
Patch v1.0 Proposed fix.

> +pref("mail.biff.alert.show_preview", true);
> +pref("mail.biff.alert.show_subject", true);
> +pref("mail.biff.alert.show_sender",  true);
> +pref("mail.biff.alert.preview_length", 40);

These preferences should eventually be moved to shared /mailnews and removed from Thunderbird but not until we get the flying mail toasters.

> +.folderSummary-previewText {
> +/*
> +  color: grey;          #808080
> +         Indigo;        #4B0082
> +         DimGray;       #696969
> +         DarkSlateGray; #2F4F4F
> +*/
> +  color: DimGray;

Thunderbird uses Grey/Gray but I think it's too light. I'm rather partial to Indigo, but I expect that's just me. I think DimGray is dark enough to be read without eye strain.


> +.folderSummary-previewText {
> +  color: #696969; /* DimGray; */
> +}
Either way up/Your place or mine!
Comment 3 Philip Chee 2012-06-26 13:27:20 PDT
Comment on attachment 636161 [details] [diff] [review]
Patch v1.0 Proposed fix.

Jens did most of the groundwork in Bug 174234, so asking him for feedback.
Comment 4 Jens Hatlak (:InvisibleSmiley) 2012-06-26 14:36:15 PDT
Comment on attachment 636161 [details] [diff] [review]
Patch v1.0 Proposed fix.

Folder popup works, colors/contrast are fine with me, traditional "toaster" biff popups still work. f=me (only tested on Win7).

You should probably add some comment accompanying the new prefs to explain we don't have full support yet and want to move them to mailnews/ eventually.

Possible follow-ups:
* sync the new prefs (add a comment/dependency to bug 728949)
* add to Preferences (new bug)
* adapt Help (new bug)
Comment 5 Stefan [:stefanh] 2012-07-01 09:52:16 PDT
Comment on attachment 636161 [details] [diff] [review]
Patch v1.0 Proposed fix.

Looks good on Mac (10.7). I agree that DimGray looks better than gray
Comment 6 neil@parkwaycc.co.uk 2012-07-02 14:39:59 PDT
Comment on attachment 636161 [details] [diff] [review]
Patch v1.0 Proposed fix.

>-          var popupValue = null;
>+          var popupValue;
Nit: unnecessary.

>-            let tooltip = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "label");
>-            tooltip.setAttribute("value", popupValue);
>-            tooltip.className = "tooltip-label";
>+            let loc = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "folderSummaryLocation");
>+            loc.setAttribute("location", popupValue);
>             let folderSummary = document.getAnonymousNodes(this)[0];
>-            document.getAnonymousNodes(folderSummary)[0].appendChild(tooltip);
>+            document.getAnonymousNodes(folderSummary)[0].appendChild(loc);
Why this change?

>+.folderSummary-previewText {
>+  color: DimGray;
Had you asked me I would have suggested GrayText for Classic and #8C99AB for Modern.
I guess I'd better try them out to see what they look like ;-)
Comment 7 Philip Chee 2012-07-05 09:12:06 PDT
Created attachment 639368 [details]
Screenshot of folder name as tooltip + summaries.

>>-          var popupValue = null;
>>+          var popupValue;
> Nit: unnecessary.
Fixed locally.

>>-            let tooltip = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "label");
>>-            tooltip.setAttribute("value", popupValue);
>>-            tooltip.className = "tooltip-label";
>>+            let loc = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "folderSummaryLocation");
>>+            loc.setAttribute("location", popupValue);
>>             let folderSummary = document.getAnonymousNodes(this)[0];
>>-            document.getAnonymousNodes(folderSummary)[0].appendChild(tooltip);
>>+            document.getAnonymousNodes(folderSummary)[0].appendChild(loc);
> Why this change?
Hmm. I've reverted the change and retested. The tooltip doesn't align with the folder summaries, unless I remove the "tooltip-label" classname.

>>+.folderSummary-previewText {
>>+  color: DimGray;
> Had you asked me I would have suggested GrayText for Classic and #8C99AB for Modern.
> I guess I'd better try them out to see what they look like ;-)
And?
Comment 8 Philip Chee 2012-07-05 10:05:36 PDT
Created attachment 639385 [details] [diff] [review]
Patch v1.1 back to using a label but without .tooltip-label class

> Hmm. I've reverted the change and retested. The tooltip doesn't align with the
> folder summaries, unless I remove the "tooltip-label" classname.
Comment 9 neil@parkwaycc.co.uk 2012-07-05 12:44:15 PDT
Comment on attachment 639385 [details] [diff] [review]
Patch v1.1 back to using a label but without .tooltip-label class

>+          return !!popupValue || newMessages;
Does this not work without the !! ?

>+  <binding id="folderSummary-location">
Do we still need this?

>+folderSummaryLocation
Or this?

>+.folderSummary-previewText {
>+  color: #696969; /* DimGray; */
>+}
As I recall, I liked my colours. Sorry for mentioning it before.
Comment 10 Stefan [:stefanh] 2012-07-05 12:59:31 PDT
I haven't tested, but on Mac, GrayText equals to NSColor disabledControlTextColor. Not sure that's the right text color here.
Comment 11 Philip Chee 2012-07-06 03:42:36 PDT
Created attachment 639619 [details] [diff] [review]
Patch v1.2 Fix Nits.

>>+          return !!popupValue || newMessages;
> Does this not work without the !! ?
Hmm. So it does.

>>+  <binding id="folderSummary-location">
> Do we still need this?
No. reverted.

>>+folderSummaryLocation
> Or this?
No. reverted.

>>+.folderSummary-previewText {
>>+  color: #696969; /* DimGray; */
>>+}
> As I recall, I liked my colours. Sorry for mentioning it before.
Fixed.

> Had you asked me I would have suggested GrayText for Classic and #8C99AB for Modern.
> I guess I'd better try them out to see what they look like ;-)

> Stefan [:stefanh]      2012-07-05 12:59:31 PDT
> 
> I haven't tested, but on Mac, GrayText equals to NSColor disabledControlTextColor.
> Not sure that's the right text color here.
Suggestions needed.
Comment 12 neil@parkwaycc.co.uk 2012-07-06 04:23:41 PDT
(In reply to Stefan from comment #10)
> Not sure that's the right text color here.

.menulist-description uses GrayText too...
Comment 13 Stefan [:stefanh] 2012-07-06 07:12:13 PDT
(In reply to neil@parkwaycc.co.uk from comment #12)
> (In reply to Stefan from comment #10)
> > Not sure that's the right text color here.
> 
> .menulist-description uses GrayText too...

Sure, but the tooltip background is yellow. I'll test this later on today, then we'll see.
Comment 14 Stefan [:stefanh] 2012-07-06 10:31:26 PDT
(In reply to Stefan [:stefanh] from comment #13)
> Sure, but the tooltip background is yellow. I'll test this later on today,
> then we'll see.

Turns out that GrayText is 7F7F7F on Mac. I expected the differences to be much more visible, but in reality, on the yellow background, the differences between the colors are minor. Gray and GrayText looks about the same, DimGray is imo a bit better to the eye - but GrayText or Gray looks ok too.
Comment 15 Karsten Düsterloh 2012-07-06 12:09:59 PDT
Comment on attachment 639619 [details] [diff] [review]
Patch v1.2 Fix Nits.

Cool! 
(It only took me a while to realize that I need new stuff in the folder to cause the popup. ^_^)
Comment 16 Philip Chee 2012-07-07 02:33:26 PDT
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/ad15062dae0b

Note You need to log in before you can comment on or make changes to this bug.