Last Comment Bug 397197 - Logically group items on "junk settings" panel
: Logically group items on "junk settings" panel
Status: RESOLVED FIXED
: uiwanted, ux-visual-hierarchy
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- minor with 1 vote (vote)
: Thunderbird 15.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: junktracker 729147 742503 750849
  Show dependency treegraph
 
Reported: 2007-09-22 14:04 PDT by Wayne Mery (:wsmwk, NI for questions)
Modified: 2012-05-03 11:40 PDT (History)
14 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (8.67 KB, patch)
2012-04-03 14:18 PDT, :aceman
iann_bugzilla: review+
vseerror: feedback+
Details | Diff | Splinter Review
junk settings screen shot & design proposal (31.38 KB, image/png)
2012-04-03 19:41 PDT, Wayne Mery (:wsmwk, NI for questions)
no flags Details
patch v2 (17.50 KB, patch)
2012-04-14 06:35 PDT, :aceman
bwinton: ui‑review+
Details | Diff | Splinter Review
screenshot of the patch v2 in action (106.90 KB, image/png)
2012-04-14 06:41 PDT, :aceman
no flags Details
patch v3 (17.68 KB, patch)
2012-04-18 13:25 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v4 (18.14 KB, patch)
2012-04-18 14:15 PDT, :aceman
mconley: review+
bwinton: ui‑review+
standard8: feedback-
Details | Diff | Splinter Review
patch v5 (18.22 KB, patch)
2012-04-24 14:24 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description Wayne Mery (:wsmwk, NI for questions) 2007-09-22 14:04:23 PDT
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 Worcester12345 2008-02-01 12:37:42 PST
Duplicate to Bug 323159 – "Trust junk mail headers set by:" should be consistent in UI and log location/access

?
Comment 2 Wayne Mery (:wsmwk, NI for questions) 2008-02-01 12:44:05 PST
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 ovidiu 2008-05-19 09:45:10 PDT
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 ovidiu 2008-05-19 09:47:57 PDT
oh, sorry, also bug 232486 may be of interest if not set some dep
Comment 5 Worcester12345 2011-05-11 08:56:11 PDT
Getting closer, thank you.
Comment 6 :aceman 2012-02-08 10:50:07 PST
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.
Comment 7 :aceman 2012-04-03 14:18:53 PDT
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.
Comment 8 Wayne Mery (:wsmwk, NI for questions) 2012-04-03 19:41:28 PDT
Created attachment 612072 [details]
junk settings screen shot & design proposal
Comment 9 Wayne Mery (:wsmwk, NI for questions) 2012-04-03 19:43:53 PDT
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+
Comment 10 :aceman 2012-04-03 23:41:09 PDT
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).
Comment 11 Wayne Mery (:wsmwk, NI for questions) 2012-04-04 12:55:27 PDT
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.
Comment 12 :aceman 2012-04-04 13:12:05 PDT
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.
Comment 13 Wayne Mery (:wsmwk, NI for questions) 2012-04-04 14:22:31 PDT
(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. :)
Comment 14 Ian Neal 2012-04-08 15:40:58 PDT
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.
Comment 15 :aceman 2012-04-10 09:38:08 PDT
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.
Comment 16 Wayne Mery (:wsmwk, NI for questions) 2012-04-10 09:51:55 PDT
(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
Comment 17 :aceman 2012-04-14 06:35:52 PDT
Created attachment 615041 [details] [diff] [review]
patch v2
Comment 18 :aceman 2012-04-14 06:41:03 PDT
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 19 Blake Winton (:bwinton) (:☕️) 2012-04-18 12:29:16 PDT
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.
Comment 20 :aceman 2012-04-18 13:25:38 PDT
Created attachment 616265 [details] [diff] [review]
patch v3

Then try this.
Comment 21 :aceman 2012-04-18 14:15:00 PDT
Created attachment 616285 [details] [diff] [review]
patch v4

Fix description margin for Mac.
Comment 22 Blake Winton (:bwinton) (:☕️) 2012-04-18 14:26:35 PDT
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!
Comment 23 Mike Conley (:mconley) - (Needinfo me!) 2012-04-24 07:17:41 PDT
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.
Comment 24 Mark Banner (:standard8) 2012-04-24 07:41:36 PDT
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.
Comment 25 :aceman 2012-04-24 11:20:09 PDT
Yes, that is the drill, sorry for forgetting that.
Comment 26 :aceman 2012-04-24 14:24:10 PDT
Created attachment 618042 [details] [diff] [review]
patch v5

Done.
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-04-24 16:24:25 PDT
http://hg.mozilla.org/comm-central/rev/6e2296b9590e
Comment 28 Vlado Valastiak [:wladow] @ Mozilla.sk 2012-05-01 11:39:44 PDT
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
Comment 29 :aceman 2012-05-01 11:49:16 PDT
Is this the convention here? Must the accesskey have the same key as the label?
Comment 30 Mike Conley (:mconley) - (Needinfo me!) 2012-05-01 11:59:14 PDT
Ouch, yes, it is.  Sorry for missing that.
Comment 31 Vlado Valastiak [:wladow] @ Mozilla.sk 2012-05-01 12:00:02 PDT
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.
Comment 32 :aceman 2012-05-01 12:07:30 PDT
OK, can I attach it here or a new bug?
Comment 33 Mike Conley (:mconley) - (Needinfo me!) 2012-05-01 12:11:36 PDT
A follow-up in this bug is probably fine.
Comment 34 :aceman 2012-05-03 11:40:29 PDT
Hm, why did I create a new bug then? :) Maybe the "follow-up" and "in this bug" were conflicting :)

Note You need to log in before you can comment on or make changes to this bug.