326076 - Use menulist for server secure connection (instead of radiogroup)

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Account Configuration, enhancement)

Description

[Giacomo Magnini]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060206 SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060206 SeaMonkey/1.5a

The Server Settings panel in the MailNews Account Preferences is a little crowded, especially for POP settings. This makes finding suitable accesskeys for all of the items very difficult if possible at all.
Moving the Secure Connection radiogroup to a menulist will simplify the whole panel and will save 3 accesskeys for better use. It also makes the panel less crowded. 

Reproducible: Always

Steps to Reproduce:
1. Open MailNews Account prefs
2. Go to Server Settings panel
3. See Security Settings box

Comment 1

[Giacomo Magnini]

Attachment: Screenshot of suggested change

Comment 2

[Giacomo Magnini]

Attachment: Tentative patch.

This seems to work graphically, but I'm not really sure it works fine regarding to prefs setting values, etc. (ie, not sure it actually WORKS! :) ). Someone more experienced than me should really take a look at this.

Comment 3

[[e-mail]]

Comment on attachment 210858 [details] [diff] [review]
Tentative patch.

>-    <vbox align="start" hidefor="nntp,movemail">
>+        <hbox align="center" hidefor="nntp,movemail">
Your indentantion seems to be somewhat off ;-)

>+          <label value="&socketType.label;" accesskey="&socketType.accesskey;"/>
>+              <!-- control="identity.smtpServerKey" -->
You'll want this, but with control="server.socketType"

>+              <!-- orient="horizontal" class="indent" -->
Unnecessary.

>+              <menuitem value="0" label="&neverSecure.label;" id="useDefaultItem"/>
>+              <menuitem value="1" label="&sometimesSecure.label;" id="sometimesSecure"/>
>+              <menuitem value="2" label="&alwaysSecure.label;" id="alwaysSecure"/>
>+              <menuitem value="3" label="&alwaysSSL.label;" id="alwaysSSL"/>
Note: the ids are obsolete. Historically the pref code wasn't able to set the value directly, instead it had to retrieve the element by id.

Comment 4

[Giacomo Magnini]

Attachment: Second attempt.

Tried to fix all of Neil's comments. Hopefully it is a little bit nicer, but still not sure if it works! :)

Comment 5

[Ian Neal]

Does this not conflict with bug 313245?

Comment 6

[Giacomo Magnini]

(In reply to comment #5)
> Does this not conflict with bug 313245?

Not really, IMO.
This can complement the other bug (that is, we can get this patch and fix also the other bug using this), or we can take this smaller solution instead of going the longer route as the other bug.
Or just tell me this is totally wrong and I'll try to work only on the other bug (probably in steps, since those panels are a total mess of overlays, hidden for and god-knows-what-they-are-for flags! :) ).

Comment 7

[Tony Mechelynck [:tonymec]]

Giacomo, are you still working on this (after two years)?

Comment 8

[Tony Mechelynck [:tonymec]]

Comment on attachment 210872 [details] [diff] [review]
Second attempt.

see comment #4 and comment #7

Neil, if you aren't reviewing this kind of patches anymore, please nominate another reviewer, unless Giacomo doesn't answer "yes" to comment #7

Comment 9

[[e-mail]]

Comment on attachment 210872 [details] [diff] [review]
Second attempt.

Unfortunately I can't review this because the patch has bitrotted; the .xul changes won't patch, and the .dtd file has since moved and forked into two.

Comment 10

[Giacomo Magnini]

If you want an updated pach, I can whip it up later today.

Comment 11

[Giacomo Magnini]

Attachment: Updated to tip. This will also fix bug 313245, at least the part regarding duplicated accesskeys

Updated to tip, SM only.
If the review is ok, I'll prepare another patch covering TB as well.

Comment 12

[[e-mail]]

Comment on attachment 311609 [details] [diff] [review]
Updated to tip. This will also fix bug 313245, at least the part regarding duplicated accesskeys

> <!ENTITY headersOnly.label "Fetch headers only">
>-<!ENTITY headersOnly.accesskey "f">
>+<!ENTITY headersOnly.accesskey "h">
> <!ENTITY deleteByAgeFromServer.label "For at most">
>-<!ENTITY deleteByAgeFromServer.accesskey "o">
>+<!ENTITY deleteByAgeFromServer.accesskey "t">
Given the above change, why not use "F"?

Also, is it worth shrinking the account manager height slightly?

Comment 13

[[e-mail]]

Comment on attachment 311609 [details] [diff] [review]
Updated to tip. This will also fix bug 313245, at least the part regarding duplicated accesskeys

> <!ENTITY headersOnly.label "Fetch headers only">
>-<!ENTITY headersOnly.accesskey "f">
>+<!ENTITY headersOnly.accesskey "h">
> <!ENTITY deleteByAgeFromServer.label "For at most">
>-<!ENTITY deleteByAgeFromServer.accesskey "o">
>+<!ENTITY deleteByAgeFromServer.accesskey "t">
Given the above change, why not use "F"?

Also, is it worth shrinking the account manager height slightly?

Comment 14

[Giacomo Magnini]

(In reply to comment #12)
> Given the above change, why not use "F"?

Because "f" is already used for "Set as Default" (lower left, under the accounts tree).

> Also, is it worth shrinking the account manager height slightly?

Yes, this is probably worth it.

Comment 15

[[e-mail]]

(In reply to comment #14)
> (In reply to comment #12)
> > Given the above change, why not use "F"?
> Because "f" is already used for "Set as Default" (lower left, under the
> accounts tree).
Then you might want to change "newrsc file" from "f" to "n".

> > Also, is it worth shrinking the account manager height slightly?
> Yes, this is probably worth it.
FYI this is (mac)AccountManager.size in AccountManager.dtd (note that it would be nice if we can get away with one width for all platforms in ch units).

Comment 16

[Giacomo Magnini]

Attachment: WIP patch, ak problems sorted

This should solve all accesskey related problems, and reduce heigth of the account window to 45em on all platforms. I'll need someone on Win/Mac to test this: Neil? Stefan?
If this works, we can remove the mac only entity.

Comment 17

[Magnus Melin [:mkmelin]]

Re getting the width ot be the same on all platforms: xref bug 317659. (On linux it would need to be wider than it is currently.)

Comment 18

[Giacomo Magnini]

I've experimented a bit and found that 65ch for width of the Account Window looks good, with 17ch as the width of the accounts tree.
Magnus, Karsten, Stefan, Neil: would you be able to test under Mac/Win and TB?
I'm starting my first ever TB build now under Linux to test it myself in such configuration.
Seems like we can't use ch for height, right?

Comment 19

[Magnus Melin [:mkmelin]]

Only linux for me.

Comment 20

[Giacomo Magnini]

To prevent KaiRo, changed locally an accesskey from "k" to "J"... ;)

Comment 21

[[e-mail]]

(In reply to comment #18)
>Seems like we can't use ch for height, right?
ch makes no sense for height. (em never really make sense for width...)

Comment 22

[Giacomo Magnini]

Attachment: Updated patch with em->ch for testing under other paltforms

Comment 23

[Stefan [:stefanh]]

Comment on attachment 312277 [details] [diff] [review]
Updated patch with em->ch for testing under other paltforms

-<!ENTITY accountManager.size "width: 55em; height: 50em;">
-<!ENTITY macAccountManager.size "width: 60em; height: 50em;">
-<!ENTITY accountTree.width   "width: 17em;">
+<!ENTITY accountManager.size "width: 65ch; height: 45em;">
+<!ENTITY macAccountManager.size "width: 65ch; height: 45em;">
+<!ENTITY accountTree.width   "width: 17ch;">

I'm not saying it's bad to try, but doesn't this assume that all platforms have the same margin/padding/size of widgets in their windows?

Anyway, on mac, the account tree becomes too narrow. Also, I think the width of the accountmanager needs to be increased a bit. The height was ok before, I think. 45em makes the "Advanced..." button in am-server.xul almost touch the OK/Cancel buttons...

Comment 24

[[e-mail]]

(In reply to comment #23)
> I'm not saying it's bad to try, but doesn't this assume that all platforms have
> the same margin/padding/size of widgets in their windows?
For all themes too. Not much you can do about that really, but it would be good if we can come up with some figures that look good everywhere.

Comment 25

[Giacomo Magnini]

Stefan, Neil: what about width: 70ch; height: 47em; for Acct Window and 20ch; for Acct tree?

Comment 26

[Stefan [:stefanh]]

(In reply to comment #25)
> Stefan, Neil: what about width: 70ch; height: 47em; for Acct Window and 20ch;
> for Acct tree?
> 

A tree width of 20ch is ok - it makes the tree a couple of px wider than before. The 70ch accountManager width is also ok.

Speaking of the height: Sorry, I didn't realized that even 50em was not enough because I forgot to check how it looked before I added your changes (and 45em just cuts out the "Local Directory" section completely). We need 52em for the height... We might be able to use 50em for the height once the dialogheader disappears (see bug 384080).

Comment 27

[Karsten Düsterloh]

Comment on attachment 312277 [details] [diff] [review]
Updated patch with em->ch for testing under other paltforms

r=me with the Mac height set to 52em (actually, Modern might get away with 50em, but Classic doesn't).

Comment 28

[Giacomo Magnini]

Attachment: Updated for review comments

Carrying over r+, asking sr?

Comment 29

[Giacomo Magnini]

Attachment: v3. No changes, just added the equivalent ak changes for TB

Seems like I can't carry over sr...

Comment 30

[Magnus Melin [:mkmelin]]

So, this patch is mainly just to make it easier to find access keys? In a way it just makes it harder to see what you can choose from IMO, and the label "Use secure connection" is a bit odd for all the options except "Never". On the plus side, saving horizontal space could be needed if we fix bug 350314...

Comment 31

[Magnus Melin [:mkmelin]]

On the other hand, it's a good step towards the version Clark has some wiki page about, can't find it atm, with a checkbox to enable the connection security menu at all, dropping the None option. That idea I like very much.

Comment 32

[Phil Ringnalda (:philor)]

Hey, I bet I know why Bryan hasn't commented about whether or not this is going in the direction he wants: it's probably because he isn't cc:ed.

Comment 33

[[e-mail]]

(In reply to comment #31)
> On the other hand, it's a good step towards the version Clark has some wiki
> page about, can't find it atm, with a checkbox to enable the connection
> security menu at all, dropping the None option. That idea I like very much.
You might find you have to rework the backend to achieve that, since currently the connection type is a single integer preference.

Comment 34

[Bryan Clark (DevTools PM) [:clarkbw]]

Oh, hello :)  Here's the wiki page:
http://wiki.mozilla.org/MailNews:Account_Wizard:Email#Page_3

This is the layout I wanted to see -

  [ ] Use Secure Connection [  TLS, if available \/]

- Removing the "Never" option and replace it with a check box for turning on/off secure connections.  Never is a bit of a scary term for many users and could likely make them want to look into the other selections, none of which are understandable opposites (i.e. sometimes, always).

Comment 35

[[e-mail]]

(In reply to comment #34)
>   [ ] Use Secure Connection [  TLS, if available \/]
Actually the UI does not count this as a secure connection!

Comment 36

[Magnus Melin [:mkmelin]]

Then again, would be nice to get rid of that option altogether - see bug 311657 comment 33 for some discussion. (If only we could auto-probe and automatically upgrade those who use it.)

Comment 37

[Bryan Clark (DevTools PM) [:clarkbw]]

- some updates -

The Account Wizard wiki has been updated to reflect some discussion coming out of the wiki talk page.

Also bug 422814 has some work on auto-probe for connection type and port config

Comment 38

[Tony Mechelynck [:tonymec]]

Giacomo: Feel free to reassign this bug to yourself if still interested.

Comment 39

[Magnus Melin [:mkmelin]]

This got fixed in bug 350314 as the possible "STARTTLS, if available" and the other options would have become too wide as radio buttons.