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

RESOLVED FIXED

Status

SeaMonkey
MailNews: Message Display
--
enhancement
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Daniel Wang, Assigned: Stefan Borggraefe)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

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

Comment 1

14 years ago
Created attachment 139435 [details]
UI mockup
(Assignee)

Comment 2

14 years ago
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.
(Assignee)

Comment 3

14 years ago
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.
(Assignee)

Updated

14 years ago
Depends on: 235860

Comment 4

13 years ago
xref bug 234121
Product: Browser → Seamonkey
(Assignee)

Comment 5

13 years ago
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.
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)
(Assignee)

Comment 6

13 years ago
Created attachment 171692 [details]
Screenshot of the new patch in action

Comment 7

13 years ago
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

13 years ago
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-
(Assignee)

Comment 9

13 years ago
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.
(Assignee)

Comment 10

13 years ago
Created attachment 171702 [details] [diff] [review]
Patch V2.1
Attachment #171691 - Attachment is obsolete: true
Attachment #171692 - Attachment is obsolete: true

Comment 11

13 years ago
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
(Assignee)

Comment 12

13 years ago
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?
Attachment #171702 - Attachment is obsolete: true
Attachment #171734 - Flags: review?(neil.parkwaycc.co.uk)

Comment 13

13 years ago
Stefan B,

Can you update Help as well, please?

Comment 14

13 years ago
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+
(Assignee)

Comment 15

13 years ago
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)
(Assignee)

Updated

13 years ago
No longer depends on: 235860

Updated

13 years ago
Attachment #171734 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Updated

13 years ago
Blocks: 279031
(Assignee)

Comment 16

13 years ago
Fix checked in. I filed bug 279031 for the Help update.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 17

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

Comment 18

13 years ago
Yes, please add an L10n note for that split entity, so that localizers have a
clue what they're dealing with...
(Assignee)

Comment 19

13 years ago
Created attachment 171818 [details] [diff] [review]
Adds the l10n note
Attachment #171818 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #171818 - Flags: review?(neil.parkwaycc.co.uk)

Comment 20

13 years ago
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+
(Assignee)

Comment 21

13 years ago
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)

Comment 22

13 years ago
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 :)

Updated

13 years ago
Attachment #171818 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 23

13 years ago
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.