Closed Bug 231428 Opened 19 years ago Closed 18 years ago

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

Categories

(SeaMonkey :: MailNews: Message Display, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: danielwang, Assigned: Stefan.Borggraefe)

References

Details

Attachments

(3 files, 6 obsolete files)

UI for "mark messages read after displaying for ___ seconds"
bug 75866
Attached image UI mockup (obsolete) —
Attached image Screenshot (obsolete) —
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.
Attached patch The patch (this does not fit!) (obsolete) — Splinter Review
This is the patch that results in attachment 141956 [details]. This works, but the option
does not fit into the pane.
Depends on: 235860
Product: Browser → Seamonkey
Attached patch Patch V2.0 (obsolete) — Splinter Review
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.
Assignee: mscott → Stefan.Borggraefe
Attachment #139435 - Attachment is obsolete: true
Attachment #141956 - Attachment is obsolete: true
Attachment #141957 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #171691 - Flags: review?(neil.parkwaycc.co.uk)
Attached image Screenshot of the new patch in action (obsolete) —
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 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 ;-)
Attachment #171691 - Flags: review?(neil.parkwaycc.co.uk) → review-
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.
Attached patch Patch V2.1 (obsolete) — Splinter Review
Attachment #171691 - Attachment is obsolete: true
Attachment #171692 - Attachment is obsolete: true
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
Attached patch Patch V2.2Splinter Review
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?
Attachment #171702 - Attachment is obsolete: true
Attachment #171734 - Flags: review?(neil.parkwaycc.co.uk)
Stefan B,

Can you update Help as well, please?
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.
Attachment #171734 - Flags: review?(neil.parkwaycc.co.uk) → review+
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.
Attachment #171734 - Flags: superreview?(bienvenu)
No longer depends on: 235860
Attachment #171734 - Flags: superreview?(bienvenu) → superreview+
Blocks: 279031
Fix checked in. I filed bug 279031 for the Help update.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
I just realised that the split of "Wait" and "minutes" should be l10n noted...
Yes, please add an L10n note for that split entity, so that localizers have a
clue what they're dealing with...
Attachment #171818 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #171818 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 171818 [details] [diff] [review]
Adds the l10n note

Yes, this looks good to me, thanks...
Attachment #171818 - Flags: review?(neil.parkwaycc.co.uk) → review+
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...
Attachment #171818 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview?(bienvenu)
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 :)
Attachment #171818 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 171818 [details] [diff] [review]
Adds the l10n note

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