Closed Bug 456872 Opened 16 years ago Closed 16 years ago

create Advanced -> Display preference tab; move all relevant preferences to it

Categories

(Thunderbird :: Preferences, enhancement)

Product:
Thunderbird
Component:
Preferences
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 3.0b1

People

(Reporter: clarkbw, Assigned: clarkbw)

References

(
URL
)

Details

Attachments

(2 files, 3 obsolete files)

advanced -> reading & display pref tab (ss)
51.62 KB, image/png
Details
patch creates adv -> reading & display tab w/ grouping v3
8.89 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review 
Currently the Advanced -> General tab has mostly message display related preferences sitting in it.  We need to move these preference types out of the General tab and into their own Display tab as we need space in the General tab.

This will help resolve the following bugs
bug 452909
bug 452890
Attached image advanced -> display pref tab (obsolete) — 
here's a screenshot of the change.  Moving all the elements except for the return receipts and config editor out of the General tab.
Assignee: nobody → clarkbw
Attached patch patch creates adv -> display tab (obsolete) — — Splinter Review 
here's the patch that makes the change happen.
(In reply to comment #1)
> Created an attachment (id=340255) [details]
> advanced -> display pref tab
> 
> here's a screenshot of the change.  Moving all the elements except for the
> return receipts and config editor out of the General tab.

I fully agree with this revision.

While on the general topic of the Advance tab, the Text label for Return Receipt could use some cleanup. Suggest "Set Global Return Receipt Policy" and we can remove a Branding issue at the same time.
Looks like a good plan, but i wonder if we can come up with a better name than "Display". Since we have the main level "Display" already I can just imagine the joy of trying to explain to someone where this is... No good suggestions though.
How about "layout" or "interface" instead of "Display"?
(In reply to comment #4)
> Looks like a good plan, but i wonder if we can come up with a better name than
> "Display". Since we have the main level "Display" already I can just imagine
> the joy of trying to explain to someone where this is... No good suggestions
> though.

Looked at the screenshot and "Chrome" came to mind. I agree that dual use of any Display related alternatives will present difficulties.
What about "Interface" for the main tab, and "Functionality" for the sub-tab?
(In reply to comment #4)
> [...] i wonder if we can come up with a better name than
> "Display". Since we have the main level "Display" already [...]

True, but we also already have "General" at the main level and also as a tab inside "Advanced".  This seems to work okay so maybe it's not a problem to do the same for "Display".

Perhaps people who use the Advanced section are advanced enough to understand the distinction.  :)
(In reply to comment #8)
> True, but we also already have "General" at the main level and also as a tab
> inside "Advanced".  This seems to work okay so maybe it's not a problem to do
> the same for "Display".

The top level "General" is getting rebranded to "Main".
"Reading" perhaps?

And while we're at it, would it make sense change the order a bit and put the "show" items next to each other?
> "Reading" perhaps?

Maybe "Reading & Display" ?

> And while we're at it, would it make sense change the order a bit and put the
> "show" items next to each other?

Good point.  I'm not sure if R & D is correct but your comment reminded me that we could group the options with these kinds of groupings.

.--Reading-------------.
| Mark Messages...     |
| Open in...           |
'----------------------'

.--Display-------------.
| Show only...         |
| Show expanded...     |
'----------------------'
A good idea to clarify to the User the reason those items are present in the Dialog.
here's an updated patch with suggested changes.  screenshot coming
Attachment #340256 - Attachment is obsolete: true
here's the updated screenshot
Attachment #340255 - Attachment is obsolete: true
Looks good.  What will the revised General Tab look like?
With this patch General only holds Return Receipts and Advanced Editor (about:config); it looks a bit sparse.

In bug 452890 comment #5 i mentioned that we should probably add some grouping to the general tab to clean it up while landing both bug 452909 and bug 452890
I added a comment to Bug 452909 since it addresses new items for the Advanced > General tab.
Couldn't "Immediately on display" be removed?

"After delaying for "0" seconds" == "Immediately on display" ?



if that's so, I guess both radio buttons could be depricated, and the UI would become:

Reading
  [v] Automatically mark messages as read after [    0] seconds.

  Open Messages in:
    ( ) ...
good points, but i'm pretty sure i've seen some large discussions on that pref before.  I think it's bug 234121 but i'm sure there was another as well.  Either way it's a separate issue I don't think we want to tackle here.
Isn't bug 234121 about that it was proposed that "zero" should mean disable?

If I have understood the problem correct, then that is not the same scenario here.

Here the user check marks "Automatically mark messages" and it is first then that "as read after [    0] seconds" is evoked. So it is not a matter of giving "zero" a special meaning. Zero means zero in this case =)

If you think this have ground, please let me know, and I will file a bug for it, otherwise I will let it be.

Btw. I don't understand the purpose of this option. Why would anyone want to delay "mark as read"?
(In reply to comment #20)
> Btw. I don't understand the purpose of this option. Why would anyone want to
> delay "mark as read"?

I believe this entire feature was added for OE parity.  IIUC, in OE it's purpose was to prevent execution of scripts while permitting a preview of text. That it also would auto open the message in the OE message pane at the end of the time delay. I see no need for the feature since Tb has not needed a security preview. It is more an OE migration comfort zone accommodation.

However in light of recent discoveries on trunk, We may want to keep this and adopt the same strategy that MS did in OE to mitigate JS exposure and use a timed preview where this feature defaults to 5-10 seconds.

Therefore, if this gets hidden right now and space is reserved for later reactivation, we get a UI cleanup in the short term.
Wow! It sounds like a dirty MS work around, instead of fixing the actual problem =(

It sounds like having a time out is meant for people in the old days, where a regular user would get 2-3 emails per day.

> I see no need for the feature since Tb has not needed a
> security preview. It is more an OE migration comfort zone accommodation.

In that case, I really think it should be removed, as there have been put a lot of effort in to clearing up the Preferences menu in bug 448716. Finding an opposite feature is a great accomplishment =)

> However in light of recent discoveries on trunk, We may want to keep this and
> adopt the same strategy that MS did in OE to mitigate JS exposure and use a
> timed preview where this feature defaults to 5-10 seconds.

Given that to days users get many emails a day, and most of them are legitimate. Wouldn't it be quite annoying to have a time delay, rather than a fix?

If "Automatically mark messages..." is meant to be a security feature, wouldn't it be better to remove this from "Advanced->Reading & Display", and add a "Block Java Script" to the "Security" tab as Bryan have mocked up at

https://wiki.mozilla.org/User:Clarkbw/Preferences#Security

Maybe this can be compared to pop ups in Firefox? It wouldn't make much sense to delay pop ups, but Firefox really got momentum, when it was able to block pop ups.
(In reply to comment #22)
> If "Automatically mark messages..." is meant to be a security feature, wouldn't
> it be better to remove this from "Advanced->Reading & Display", and add a
> "Block Java Script" to the "Security" tab as Bryan have mocked up at

I detect You are not aware of a major JS security issue that was discovered. In trunk, JS is now locked out by a patch to the binaries. User choice is bypassed and allowing use of JS in the message pane is getting a top-down review.
Okay, that sounds pretty bad =(

When the JS security bug have been fixed, I think I will file a bug proposing a "Block JS" feature. Something like:

  [ ] Allow Java Script to be executed in messages.


If Bryan gives a "go for it" would it then be okay with you, that "Mark messages as..." is removed?
JavaScript in mail is disabled by default (and like Ron notes, completely disabled on trunk now). We certainly don't want to add UI to enable it in mail, enabling it for feeds (and possibly news) is another issue, but that's for later when the backend would allow such a thing.

Some people want the delay, some don't, some don't want auto-marking as read at all - it's not just OE parity (and has nothing to do with javascript execution). You can read a lengthy discussion in bug 297534. I don't think we want to remove it, nor change it (again).
The screenshot looks nice and clean, IMO!
Okay, that is one long discussion!

I wonder, what if the delay functionality remained in about:config, but the UI for it was removed, as it serves no purpose to the arrive user?

A similar solution was done in Bug 448694.
Comment on attachment 342302 [details] [diff] [review]
patch creates adv -> reading & display tab w/ grouping

magnus: care to do another pref review?
Attachment #342302 - Flags: review?(mkmelin+mozilla)
Comment on attachment 342302 [details] [diff] [review]
patch creates adv -> reading & display tab w/ grouping

>+            <vbox>
>+              <hbox align="center" pack="start">
>+                <label value="&openMsgIn.label;" control="mailnewsDoubleClick2NewWindow"/>
>+              </hbox>

Drop the "align" as it's not needed for on item. Also pack="start" is the default, so it can be dropped.

>+
>+              <hbox class="indent">
>+                <radiogroup id="mailnewsDoubleClick2NewWindow" 

Can be just <radiogroup class="indent"

>+                  <radio value="false" label="&reuseExpRadio0.label;" accesskey="&reuseExpRadio0.accesskey;" id="new"/>
>+                  <radio value="true" label="&reuseExpRadio1.label;" accesskey="&reuseExpRadio1.accesskey;" id="existing"/>

Since you're touching it, please move the id attribute first, so one doesn't have to look for it.

>+              <checkbox id="showCondensedAddresses" label="&showCondensedAddresses.label;"
>+                        accesskey="&showCondensedAddresses.accesskey;"  preference="mail.showCondensedAddresses"/>

There's some extra space before "preference"

Ok, partly you're just copying code, but you thouch it, you own it;)
Some of the lines are really long, please wrap them around ~80 chars. Esp the ones going on 120+ are hard to read :)
Ack, thanks for doing this.  As you can tell IANAP (I am not a programmer) :)

> Drop the "align" as it's not needed for on item. Also pack="start" is the
> default, so it can be dropped.

Ok, dropped both attrs.

> Can be just <radiogroup class="indent"

Changed and indented for 80ch

> Since you're touching it, please move the id attribute first, so one doesn't
> have to look for it.

Moved ids to front and indented for 80ch each

> There's some extra space before "preference"

Removed and indented for 80ch

> Ok, partly you're just copying code, but you thouch it, you own it;)

Yay! :)

> Some of the lines are really long, please wrap them around ~80 chars. Esp the
> ones going on 120+ are hard to read :)

Also indented #markAsReadAutoPreferences and #showFolderPaneColumns to fit 80ch each.

Re-requesting review
Attachment #342302 - Attachment is obsolete: true
Attachment #342521 - Flags: review?(mkmelin+mozilla)
Attachment #342302 - Flags: review?(mkmelin+mozilla)
Attachment #342521 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 342521 [details] [diff] [review]
patch creates adv -> reading & display tab w/ grouping v3

Looks good! r=mkmelin. I'll check this in shortly. (Will remove a few trailing spaces that snuck in.)

I'm a little bit concerned about localizes as the tab bar under advanced is now full... Let's hope it works out.
changeset:   575:6454eb36661a
http://hg.mozilla.org/comm-central/rev/6454eb36661a

->FIXED
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b1
We may have a regression from this patch.

Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.setIntPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://global/content/bindings/preferences.xml :: set_valueFromPreferences :: line 335"  data: no]
Source File: chrome://global/content/bindings/tabbox.xml
Line: 571

This was logged in error console when I switched tabs within the Advance tab of Options dialog. Each tab switch generated two entries. The Line: xxx of tabbox.xml changed, but the errors all point to preferences.xml Line 335
FWIW, i don't see that, but if it's reproducible for you, please file a follow-up bug.
Bug #459612 opened for issue in Comment #33
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: