231428 - UI for "mark messages read after displaying for ___ seconds"

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Daniel Wang]

UI for "mark messages read after displaying for ___ seconds"
bug 75866

Comment 1

[Tarmo Järvalt]

Attachment: UI mockup

Comment 2

[Stefan Borggraefe]

Attachment: 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]

Attachment: 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]

xref bug 234121

Comment 5

[Stefan Borggraefe]

Attachment: 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]

Attachment: Screenshot of the new patch in action

Comment 7

[[e-mail]]

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

[[e-mail]]

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]

Attachment: 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]

Attachment: Patch V2.1

Comment 11

[Ian Neal]

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]

Attachment: 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]]

Stefan B,

Can you update Help as well, please?

Comment 14

[[e-mail]]

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]

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]

Fix checked in. I filed bug 279031 for the Help update.

Comment 17

[[e-mail]]

I just realised that the split of "Wait" and "minutes" should be l10n noted...

Comment 18

[Robert Kaiser]

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]

Attachment: Adds the l10n note

Comment 20

[Robert Kaiser]

Comment on attachment 171818 [details] [diff] [review]
Adds the l10n note

Yes, this looks good to me, thanks...

Comment 21

[Stefan Borggraefe]

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]

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]

Comment on attachment 171818 [details] [diff] [review]
Adds the l10n note

Checked in.