42.03 KB, image/png
8.49 KB, patch
|Details | Diff | Splinter Review|
1.29 KB, patch
Robert Kaiser: review+
|Details | Diff | Splinter Review|
UI for "mark messages read after displaying for ___ seconds" bug 75866
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.
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.
xref bug 234121
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.
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 ;-)
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.
Created attachment 171702 [details] [diff] [review] Patch V2.1
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
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?
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.
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.
Fix checked in. I filed bug 279031 for the Help update.
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...
Created attachment 171818 [details] [diff] [review] Adds the l10n note
Comment on attachment 171818 [details] [diff] [review] Adds the l10n note Yes, this looks good to me, thanks...
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...
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 :)