397197 - Logically group items on "junk settings" panel

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Wayne Mery (:wsmwk)]

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]

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

?

Comment 2

[Wayne Mery (:wsmwk)]

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]

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]

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

Comment 5

[Worcester12345]

Getting closer, thank you.

Comment 6

[:aceman]

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]

Attachment: 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)]

Attachment: junk settings screen shot & design proposal

Comment 9

[Wayne Mery (:wsmwk)]

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]

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)]

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]

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)]

(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]

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]

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)]

(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]

Attachment: patch v2

Comment 18

[:aceman]

Attachment: 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) (:☕️)]

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]

Attachment: patch v3

Then try this.

Comment 21

[:aceman]

Attachment: patch v4

Fix description margin for Mac.

Comment 22

[Blake Winton (: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!

Comment 23

[Mike Conley (: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.

Comment 24

[Mark Banner (:standard8)]

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]

Yes, that is the drill, sorry for forgetting that.

Comment 26

[:aceman]

Attachment: patch v5

Done.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/6e2296b9590e

Comment 28

[Vlado Valastiak [:wladow]]

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]

Is this the convention here? Must the accesskey have the same key as the label?

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Ouch, yes, it is.  Sorry for missing that.

Comment 31

[Vlado Valastiak [:wladow]]

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]

OK, can I attach it here or a new bug?

Comment 33

[Mike Conley (:mconley) (:⚙️)]

A follow-up in this bug is probably fine.

Comment 34

[:aceman]

Hm, why did I create a new bug then? :) Maybe the "follow-up" and "in this bug" were conflicting :)