Last Comment Bug 231428 - UI for "mark messages read after displaying for ___ seconds"
: UI for "mark messages read after displaying for ___ seconds"
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Stefan Borggraefe
: esther
Mentors:
Depends on:
Blocks: 279031
  Show dependency treegraph
 
Reported: 2004-01-18 22:51 PST by Daniel Wang
Modified: 2005-02-12 21:28 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
UI mockup (27.79 KB, image/png)
2004-01-19 12:10 PST, Tarmo Järvalt
no flags Details
Screenshot (51.85 KB, image/png)
2004-02-22 06:00 PST, Stefan Borggraefe
no flags Details
The patch (this does not fit!) (4.86 KB, patch)
2004-02-22 06:10 PST, Stefan Borggraefe
no flags Details | Diff | Splinter Review
Patch V2.0 (7.95 KB, patch)
2005-01-18 15:38 PST, Stefan Borggraefe
neil: review-
Details | Diff | Splinter Review
Screenshot of the new patch in action (43.31 KB, image/png)
2005-01-18 15:44 PST, Stefan Borggraefe
no flags Details
Screenshot with "Viewing Messages" groupbox (42.03 KB, image/png)
2005-01-18 17:41 PST, Stefan Borggraefe
no flags Details
Patch V2.1 (8.53 KB, patch)
2005-01-18 17:46 PST, Stefan Borggraefe
no flags Details | Diff | Splinter Review
Patch V2.2 (8.49 KB, patch)
2005-01-19 02:10 PST, Stefan Borggraefe
neil: review+
mozilla: superreview+
Details | Diff | Splinter Review
Adds the l10n note (1.29 KB, patch)
2005-01-19 16:13 PST, Stefan Borggraefe
kairo: review+
mozilla: superreview+
Details | Diff | Splinter Review

Description Daniel Wang 2004-01-18 22:51:03 PST
UI for "mark messages read after displaying for ___ seconds"
bug 75866
Comment 1 Tarmo Järvalt 2004-01-19 12:10:29 PST
Created attachment 139435 [details]
UI mockup
Comment 2 Stefan Borggraefe 2004-02-22 06:00:22 PST
Created attachment 141956 [details]
Screenshot

I just tried to implement something like this. As it turns out, this new option
doesn't fit anymore into this pane. As you can see on the screenshot I already
tried to save some space by using orient="horizontal" on the radiogroup.

I think the best solution for this problem would be to implement

http://www.mozilla.org/mailnews/specs/prefs/Preferences.html#Lang

first, so the Languages options are moved to another pane.
Comment 3 Stefan Borggraefe 2004-02-22 06:10:06 PST
Created attachment 141957 [details] [diff] [review]
The patch (this does not fit!)

This is the patch that results in attachment 141956 [details]. This works, but the option
does not fit into the pane.
Comment 4 Mike Cowperthwaite 2004-09-02 11:57:55 PDT
xref bug 234121
Comment 5 Stefan Borggraefe 2005-01-18 15:38:48 PST
Created attachment 171691 [details] [diff] [review]
Patch V2.0

Ok, I managed to squeeze this new option into this pref pane. As you can see in
attachment 141956 [details] one problem was that two labels wrap the word "messages" into
a new line. So I tried to rephrase the labels a bit.

I also changed the wording for the new option to avoid the kind of confusion
that led to the report of bug 234121. It now reads

  [x] Wait ___ seconds before marking a message as read

This patch also corrects the case of all accesskeys, corrects an error in a
LOCALIZATION NOTE and converts tabs to spaces in the DTD file.
Comment 6 Stefan Borggraefe 2005-01-18 15:44:00 PST
Created attachment 171692 [details]
Screenshot of the new patch in action
Comment 7 neil@parkwaycc.co.uk 2005-01-18 16:31:26 PST
Just glanced at this and noticed that you haven't accounted for the possibility
that the delay preference has been locked, and in the case that it isn't locked
it appears to be typical to focus the text field after checking the checkbox.

IanN can probably point you to a number of variants of a suitable function...
Comment 8 neil@parkwaycc.co.uk 2005-01-18 16:43:45 PST
Comment on attachment 171691 [details] [diff] [review]
Patch V2.0

Additionally:

>+    var _elementIDs = ["mailFixedWidthMessages", "mailQuotedStyle",
>+                       "mailQuotedSize", "wrapLongLines", "displayGlyph",
>+                       "mailCitationColor", "markMessagesRead",
>+                       "markMessagesReadAfter", "sendDefaultCharsetList",
>+                       "forceCharsetOverride"];
It would be nice to have these two per line in document order.

>   <groupbox align="start">
>-    <caption label="&languagesTitle.label;"/>
>-    <description>&encoding.label;</description>
>+    <caption label="&readingMessages.label;"/>
I trifled with the idea of captioning the second groupbox "Viewing Messages" so
you could just prepend your mark message read XUL to its contents. IanN, if you
get this far, feel free to comment :-)

>+<!ENTITY displayText.label                "Preferences for displaying quoted plain text messages:">
I was toying with labelling this one "Settings for ..."

> <!ENTITY forceCharsetOverride.accesskey   "A">
...
>+<!ENTITY markMessagesRead.accesskey       "a">
Oops ;-)
Comment 9 Stefan Borggraefe 2005-01-18 17:41:05 PST
Created attachment 171701 [details]
Screenshot with "Viewing Messages" groupbox

Neil's idea from comment 8 as a screenshot. I like it. :-) This saves even more
space and has no real downsides IMHO. Note that I also changes "Character
Encoding" to "Default Character Encoding". This way the checkbox below the
drop-down box ("Apply default...") is easier to understand.
Comment 10 Stefan Borggraefe 2005-01-18 17:46:44 PST
Created attachment 171702 [details] [diff] [review]
Patch V2.1
Comment 11 Ian Neal 2005-01-18 18:11:35 PST
On such example is:
http://lxr.mozilla.org/seamonkey/source/mailnews/base/prefs/resources/content/pref-offline.xul#61
Looks a bit more straightforward and efficient than in patch v2.1
Comment 12 Stefan Borggraefe 2005-01-19 02:10:55 PST
Created attachment 171734 [details] [diff] [review]
Patch V2.2

Ok, I took Ian's function and added a third parameter for the involved textbox.
Maybe this function could be moved to a central place (probably
nsPrefWindow.js) in a follow-up bug so it can be used from all pref panes that
need it (looks like there are a few). This would avoid the code duplication. Do
you think this is a good idea?
Comment 13 Stefan [:stefanh] 2005-01-19 06:37:59 PST
Stefan B,

Can you update Help as well, please?
Comment 14 neil@parkwaycc.co.uk 2005-01-19 07:00:44 PST
Comment on attachment 171734 [details] [diff] [review]
Patch V2.2

I asked smontagu and he said he was happy with your removal of the
&encoding.label;

It looks as if you don't want to you don't have to make the font radiogroup
horizontal any more.

Note for the follow-on bug: it's not always a textbox that's enabled/disabled
or a checkbox that's enabling/disabling.
Comment 15 Stefan Borggraefe 2005-01-19 11:02:47 PST
Comment on attachment 171734 [details] [diff] [review]
Patch V2.2

Thanks for the review and for asking smontagu. :-) I think I like the
horizontal oriented radiogroup better. 

David, can you sr this patch? Thanks! :-)

stefan_h: I will file a new bug (assigned to me) for the necessary help update
when I have an sr+ for this UI-change.

As for the refactoring idea, I will investigate if it really pays off before
filing a bug about it.
Comment 16 Stefan Borggraefe 2005-01-19 14:17:41 PST
Fix checked in. I filed bug 279031 for the Help update.
Comment 17 neil@parkwaycc.co.uk 2005-01-19 15:42:56 PST
I just realised that the split of "Wait" and "minutes" should be l10n noted...
Comment 18 Robert Kaiser 2005-01-19 15:46:48 PST
Yes, please add an L10n note for that split entity, so that localizers have a
clue what they're dealing with...
Comment 19 Stefan Borggraefe 2005-01-19 16:13:41 PST
Created attachment 171818 [details] [diff] [review]
Adds the l10n note
Comment 20 Robert Kaiser 2005-01-19 16:42:40 PST
Comment on attachment 171818 [details] [diff] [review]
Adds the l10n note

Yes, this looks good to me, thanks...
Comment 21 Stefan Borggraefe 2005-01-19 16:55:34 PST
Comment on attachment 171818 [details] [diff] [review]
Adds the l10n note

Ooops, I forgot that Neil can't sr mailnews/ patches. Also I should have chosen
kairo to ask for r from the beginning. Sorry guys...
Comment 22 Robert Kaiser 2005-01-19 16:58:35 PST
Stefan, it was perfectly good to ask neil for r=, I just thought I'll r+ it ask
I looked on the patch and it's trivial and looks good to me. It's my first
r=kairo anyways - but I dare to do that on such a patch as MLP leader :)
Comment 23 Stefan Borggraefe 2005-01-20 16:22:03 PST
Comment on attachment 171818 [details] [diff] [review]
Adds the l10n note

Checked in.

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