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

RESOLVED FIXED in Thunderbird 3.0b1

Status

Thunderbird
Preferences
--
enhancement
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: clarkbw, Assigned: clarkbw)

Tracking

(Blocks: 2 bugs)

Trunk
Thunderbird 3.0b1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

10 years ago
Created attachment 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.
Assignee: nobody → clarkbw
(Assignee)

Comment 2

10 years ago
Created attachment 340256 [details] [diff] [review]
patch creates adv -> display tab

here's the patch that makes the change happen.

Comment 3

10 years ago
(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.

Comment 4

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

Comment 5

10 years ago
How about "layout" or "interface" instead of "Display"?

Comment 6

10 years ago
(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.

Comment 7

10 years ago
What about "Interface" for the main tab, and "Functionality" for the sub-tab?

Comment 8

10 years ago
(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.  :)

Comment 9

10 years ago
(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".

Comment 10

10 years ago
"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?
(Assignee)

Comment 11

10 years ago
> "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...     |
'----------------------'

Comment 12

10 years ago
A good idea to clarify to the User the reason those items are present in the Dialog.
(Assignee)

Comment 13

10 years ago
Created attachment 342302 [details] [diff] [review]
patch creates adv -> reading & display tab w/ grouping

here's an updated patch with suggested changes.  screenshot coming
Attachment #340256 - Attachment is obsolete: true
(Assignee)

Comment 14

10 years ago
Created attachment 342303 [details]
advanced -> reading & display pref tab (ss)

here's the updated screenshot
Attachment #340255 - Attachment is obsolete: true

Comment 15

10 years ago
Looks good.  What will the revised General Tab look like?
(Assignee)

Comment 16

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

Comment 17

10 years ago
I added a comment to Bug 452909 since it addresses new items for the Advanced > General tab.

Comment 18

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

Comment 19

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

Comment 20

10 years ago
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"?

Comment 21

10 years ago
(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.

Comment 22

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

Comment 23

10 years ago
(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.

Comment 24

10 years ago
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?

Comment 25

10 years ago
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).

Comment 26

10 years ago
The screenshot looks nice and clean, IMO!

Comment 27

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

Comment 28

10 years ago
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 29

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

Comment 30

10 years ago
Created attachment 342521 [details] [diff] [review]
patch creates adv -> reading & display tab w/ grouping v3

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)

Updated

10 years ago
Attachment #342521 - Flags: review?(mkmelin+mozilla) → review+

Comment 31

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

Comment 32

10 years ago
changeset:   575:6454eb36661a
http://hg.mozilla.org/comm-central/rev/6454eb36661a

->FIXED
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b1

Comment 33

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

Comment 34

10 years ago
FWIW, i don't see that, but if it's reproducible for you, please file a follow-up bug.

Comment 35

10 years ago
Bug #459612 opened for issue in Comment #33
You need to log in before you can comment on or make changes to this bug.