Logically group items on "junk settings" panel

RESOLVED FIXED in Thunderbird 15.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: wsmwk, Assigned: aceman)

Tracking

(Blocks: 2 bugs, {uiwanted, ux-visual-hierarchy})

Trunk
Thunderbird 15.0
uiwanted, ux-visual-hierarchy
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

The present layout does not accurately reflect how the functions relate to each other, and therefore do not visually aid the uniformed user as well as they could. 
Presently, "enable adaptive" slightly offset from the others and the rest equally spaced.

Two suggestions to start
1. two large logical grouping might be "selection" and "action", action being the items relating to "move new junk message to:".
2. "Trust junk mail headers set by:" is unrelated to the instructions "If enabled..." at the top of this panel.

Comment 1

10 years ago
Duplicate to Bug 323159 – "Trust junk mail headers set by:" should be consistent in UI and log location/access

?
(Reporter)

Comment 2

10 years ago
they are different, which is why I filed this bug.

you wrote in bug 323159 "this bug is about where the logging of this activity [trust] goes."

Comment 3

9 years ago
I'd say there is more to these options than just the "not obvious logic" and suggestive UI as found in bug 352428. That IMO adds more confusion ..

Comment 4

9 years ago
oh, sorry, also bug 232486 may be of interest if not set some dep

Updated

9 years ago
Flags: wanted-thunderbird3?
Blocks: 531787

Comment 5

6 years ago
Getting closer, thank you.
(Assignee)

Comment 6

5 years ago
Wayne, would you have any screenshot/mockup of what this should look like?
I think I could do something here if you mean to use some existing elements (graphical) from other panels.
Component: Account Manager → Account Manager
Keywords: uiwanted, ux-visual-hierarchy
Product: Thunderbird → MailNews Core
QA Contact: account-manager → account-manager
(Assignee)

Updated

5 years ago
Blocks: 729147
(Assignee)

Comment 7

5 years ago
Created attachment 611977 [details] [diff] [review]
patch

What about this?
Adds "classification" and "actions" groupboxes as requested.

For the second point, moves the description about junk learning to the "adaptive junk mail" enabling checkbox. And adds a description to the "Trust headers set by" checkbox.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #611977 - Flags: ui-review?(bwinton)
Attachment #611977 - Flags: review?(iann_bugzilla)
Attachment #611977 - Flags: feedback?(vseerror)
Created attachment 612072 [details]
junk settings screen shot & design proposal
Comment on attachment 611977 [details] [diff] [review]
patch

(In reply to :aceman from comment #7)
> Created attachment 611977 [details] [diff] [review]
> patch
> 
> What about this?
Darn close I'd say. Let's iterate.


> Adds "classification" and "actions" groupboxes as requested.
Groupboxes along the lines of the the mock up I've now (finally) attached?  (please forgive my crude Paint results)

as for wording,  ... "classification" seems to technical, and "actions" perhaps ambiguous (classification is also an action of sorts). may I suggest "Selection" and "Destination and Retention".


> For the second point, moves the description about junk learning to the
> "adaptive junk mail" enabling checkbox. And adds a description to the "Trust
> headers set by" checkbox.
I like it.  However, are all the possibilities "server-side" only tools?  My impression was some of these might be headers injected by a tool on the PC itself, or some non-mail server?  I could be wrong.

Lastly, I've always found the term "adaptive junk" to again be technical, and I assume not particularly understandable by the average user.  So I've added the word "automatically" in a couple places. Does it help, or make things worse?

In the end, I am flexible as to format and wording - my screen shot is mere suggestion. Improvement is the goal.  But I'd like to see a second pass at the patch to incorporate some of the above, with or without a screen shot of the resulting changes. Overall, feedback+
Attachment #611977 - Flags: feedback?(vseerror) → feedback+
(Assignee)

Comment 10

5 years ago
Thanks. Yes the screenshot represents what the patch does.
I think the patch does not indent the addressbook lists, I'll try to add it, it is nice. On the other hand, I am not sure the addressbook whitelist is really dependent on the adaptive filter or if it also applies to the "trust headers" option. If it is dependent, it should be made disabled, depending on the state of adaptive junk filtering (in bug 729147).

I like the wording of "automatic" but for somebody the headers way of classsification could be considered even more automatic (see also bug 323159 for attempt at disambiguation of logs).
I use spamassassin at work. I don't recall any situation where whitelist overrode "trust" SpamA.  IOW, it only applies to adaptive.

I don't have a better word for automatic.  Perhaps Blake will have an idea, if you choose to leave it in patch v2 - I'm OK either way.
(Assignee)

Comment 12

5 years ago
Thanks, I have filed bug 742503 for disabling of the addressbook whitelist when not needed.

If you could do more tests with spamassassin and confirm the whitelist is ignored with it, please post in that bug.
(In reply to :aceman from comment #12)
> If you could do more tests with spamassassin and confirm
> the whitelist is ignored with it, please post in that bug.

my first direct test failed - adding a blacklist item causes our server to just outright reject the message.  And further testing is moot because I'm too lame to figure out how to make a test message that spama will score as spam.

I think we can default assuming whitelist is ignored, because I have a ton of whitelist entries (going back some years) in my spama configuration.  I so assert. :)
(Assignee)

Updated

5 years ago
Blocks: 742503

Comment 14

5 years ago
Comment on attachment 611977 [details] [diff] [review]
patch

>+++ b/mail/locales/en-US/chrome/messenger/am-junk.dtd
>+<!ENTITY ispHeadersWarning.label "If enabled, &brandShortName; will automatically consider messages marked by this server-side junk classifier as junk.">
junk is repeated perhaps use this instead:
...by this server-side classifier as junk

>+++ b/mailnews/base/prefs/content/am-junk.xul

>+      <label hidden="true"  id="server.spamLevel"
>+             wsm_persist="true" pref="true" preftype="int" prefattribute="value"
>+             genericattr="true" prefstring="mail.server.%serverkey%.spamLevel"/>
As you are touching this, one attribute per line, same for the rest of the lines changed in this file.

r=me with those addressed.
Attachment #611977 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 15

5 years ago
Wayne, I propose using "external" instead of "server-side" then. I also think there can be daemons on the local machine inserting those headers.

Let's see what bwinton says to all the proposals here.
(In reply to :aceman from comment #15)
> Wayne, I propose using "external" instead of "server-side" then. I also
> think there can be daemons on the local machine inserting those headers.

precisely.
I like the proposal
(Assignee)

Comment 17

5 years ago
Created attachment 615041 [details] [diff] [review]
patch v2
Attachment #611977 - Attachment is obsolete: true
Attachment #615041 - Flags: ui-review?(bwinton)
Attachment #615041 - Flags: review?(iann_bugzilla)
Attachment #611977 - Flags: ui-review?(bwinton)
(Assignee)

Updated

5 years ago
Attachment #612072 - Attachment description: junk settings screen shot → junk settings screen shot & design proposal
(Assignee)

Comment 18

5 years ago
Created attachment 615042 [details]
screenshot of the patch v2 in action

Screenshot from Linux, where the groupboxes do not have borders, but they should be there if run on Windows. Otherwise should match Wayne's proposal + some shuffling of the word 'automatically' around.
Comment on attachment 615041 [details] [diff] [review]
patch v2

So, I like this patch, but there are a couple of tweaks I'ld like to see.


Mac: screenshot at http://dl.dropbox.com/u/2301433/Screenshots/TBNewJunk.png

On Mac, like on Linux, I think the text should line up with the checkboxes.
In the Destination box, there appear to be 13 pixels of padding before the top checkbox, but only 9 pixels below the bottom checkbox.  (In the Selection box, it looks lik 12 and 8.)  I think those should all be 13px.

And I would like a little more space in between the checkboxes and their associated descriptions.  (13 or 14px, instead of the 10px that they seem to have.  And it should still be less than the 17px between items.)


Linux:

I like it.  I think we could use a little more space to make it easier to read, but other than that, it's good.


Windows:

Still compiling…


Having said all that, and barring any crazy issues on Windows that I haven't seen yet, ui-r=me with those nits fixed.  ;)

Thanks,
Blake.
Attachment #615041 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 20

5 years ago
Created attachment 616265 [details] [diff] [review]
patch v3

Then try this.
Attachment #615041 - Attachment is obsolete: true
Attachment #616265 - Flags: ui-review?(bwinton)
Attachment #615041 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 21

5 years ago
Created attachment 616285 [details] [diff] [review]
patch v4

Fix description margin for Mac.
Attachment #616265 - Attachment is obsolete: true
Attachment #616285 - Flags: ui-review?(bwinton)
Attachment #616265 - Flags: ui-review?(bwinton)
Comment on attachment 616285 [details] [diff] [review]
patch v4

I still don't have my Windows box building, but for Mac, it looks good.  ui-r=me, and thanks for the changes!
Attachment #616285 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Updated

5 years ago
Attachment #616285 - Flags: review?(mconley)
Comment on attachment 616285 [details] [diff] [review]
patch v4

Review of attachment 616285 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good - I'm just a bit wary of those string changes.

Mark - am I correct when I say that localizers only notice string changes when the keys change?

aceman:  if the above is true, then r=me with those fixed.  If not true, r=me as is.

-Mike

::: mail/locales/en-US/chrome/messenger/am-junk.dtd
@@ +1,2 @@
>  <!ENTITY junkSettings.label "Junk Settings">
> +<!ENTITY trainingWarning.label "If enabled, you must first train &brandShortName; to identify junk mail by using the Junk toolbar button to mark messages as junk or not. You need to identify both junk and non junk messages. After that &brandShortName; will be able to mark junk automatically.">

I might be wrong here, but I believe in order for the localizers to notice this change, the string key must be changed - to something like trainingWarning.label2.

@@ +12,5 @@
>  <!ENTITY purge1.label "Automatically delete junk mail older than">
>  <!ENTITY purge1.accesskey "u">
>  <!ENTITY purge2.label "days">
>  
> +<!ENTITY whitelist.label "Do not automatically mark mail as junk if the sender is in: ">

Same as above.

::: suite/locales/en-US/chrome/mailnews/pref/am-junk.dtd
@@ +1,2 @@
>  <!ENTITY junkSettings.label "Junk Settings">
> +<!ENTITY trainingWarning.label "If enabled, you must first train &brandShortName; to identify junk mail by using the Junk toolbar button to mark messages as junk or not. You need to identify both junk and non junk messages. After that &brandShortName; will be able to mark junk automatically.">

Same as above, re: key changes.

@@ +12,5 @@
>  <!ENTITY purge1.label "Automatically delete junk mail older than">
>  <!ENTITY purge1.accesskey "u">
>  <!ENTITY purge2.label "days">
>  
> +<!ENTITY whitelist.label "Do not automatically mark mail as junk if the sender is in: ">

Same as above.
Attachment #616285 - Flags: review?(mconley)
Attachment #616285 - Flags: review+
Attachment #616285 - Flags: feedback?(mbanner)
Comment on attachment 616285 [details] [diff] [review]
patch v4

Yes, we need to change the string ids here, otherwise l10n won't notice the changes.
Attachment #616285 - Flags: feedback?(mbanner) → feedback-
(Assignee)

Comment 25

5 years ago
Yes, that is the drill, sorry for forgetting that.
(Assignee)

Comment 26

5 years ago
Created attachment 618042 [details] [diff] [review]
patch v5

Done.
Attachment #616285 - Attachment is obsolete: true
Attachment #618042 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/6e2296b9590e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: wanted-thunderbird3? → in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
Comment on attachment 618042 [details] [diff] [review]
patch v5

>diff --git a/mail/locales/en-US/chrome/messenger/am-junk.dtd b/mail/locales/en-US/chrome/messenger/am-junk.dtd
>--- a/mail/locales/en-US/chrome/messenger/am-junk.dtd
>+++ b/mail/locales/en-US/chrome/messenger/am-junk.dtd
>-<!ENTITY whitelist.label "Do not mark mail as junk if the sender is in: ">
>+<!ENTITY whitelistHeader.label "Do not automatically mark mail as junk if the sender is in: ">
> <!ENTITY whitelist.accesskey "D">
> 

whitelist.accesskey needs to be renamed to whitelistHeader.accesskey
(Assignee)

Comment 29

5 years ago
Is this the convention here? Must the accesskey have the same key as the label?
Ouch, yes, it is.  Sorry for missing that.
Yes, several l10n tools need that when checking whether appropriate accesskey has been chosen or not. If the key is different these tools are not able to pair the label and the accesskey.
(Assignee)

Comment 32

5 years ago
OK, can I attach it here or a new bug?
A follow-up in this bug is probably fine.
(Assignee)

Updated

5 years ago
Blocks: 750849
(Assignee)

Comment 34

5 years ago
Hm, why did I create a new bug then? :) Maybe the "follow-up" and "in this bug" were conflicting :)
You need to log in before you can comment on or make changes to this bug.