70623 - Mail/News account settings need to honor locked prefs.

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement, P2)

Comment 1

[eddyk]

Bug 70033 handles the general pref locking by updating the nsWidgetStateManager 
but the account settings uses a different mechanism and needs to also be updated 
to be locked-pref aware.

Comment 2

[sairuh (rarely reading bugmail)]

shouldn't this go to mailnews fe...?

Comment 3

[eddyk]

I do believe you're correct.  Trying to figure out how to change it to the 
correct component...

Comment 4

[sairuh (rarely reading bugmail)]

-> mailnews fe, i do believe. thx, eddyk!

Comment 5

[lchiang]

cc: hong.  Please reassign to your QA who is working on locked prefs.

Comment 6

[racham]

Adding myself to the cc list.

Comment 7

[chris hofmann]

lets do this in 0.9, not at the last minute in 0.8.1

Comment 8

[Hong Kwon]

reassigning qa to [e-mail].

Comment 9

[eddyk]

Attachment: Patch for a possible, not yet proposed implementation

Comment 10

[eddyk]

The first attachment is an ugly, ugly hack that I just want to put up for
reference.  I don't plan on checking this in unless nothing else works.

While it may work, it's non-maintainable.  The mechanism for disabling is fine.
 It's the way I generate the preference strings in this approach that bothers me.

In increasing level of goodness (and difficulty), I'm going to look at the
following approaches:

1) extend the idea of the first patch, but instead of generating the pref
strings in the xul code, query the appropriate object for the id -> prefstring
mapping.  This would be more maintainable and less error-prone, IMO.

2) instead of generating prefstrings on the fly in various objects, put the
prefstrings in the xul elements, much is as done currently in the main
preferences window.  This will require some type of keyword substitution, and
possibly other issues will come into play.

Comment 11

[Alec Flett]

this is an incredible hack that is going to be very difficult to maintain as we
add and remove new attributes on the servers, identities, etc.

Comment 12

[eddyk]

Attachment: new, improved approach. work in progress, need feedback.

Comment 13

[eddyk]

The second attachment is a different approach, which is hopefully a step in the
right direction of the eventual goal to fully integrate with the general pref FE
code.

Please comment on this before I go too far with it. :)
Thanks

Comment 14

[Alec Flett]

suggestion: don't create cryptic strings that must be broken up, when you have
the option of breaking them up from the very beginning
i.e. instead of 
xpref="type=string,prefstr=mail.identity.%identitykey.organization"

how about
preftype="string"
prefobjtype="identity"
prefpostfix="organization"

then you can look at each individual string in there with .getAttribute(), and
reconstruct the preferences appropriately, and you can treat each string as an
opaque value.

we have a parser, it's called xpat, and it parses xml quite nicely - take
advantage of that! :)

Comment 15

[Blake Ross]

Sorry about that, that's my fault.  I proposed that we investigate alternate 
ways of doing this instead of adding more and more pref* attributes.  I'm 
getting tired of seeing:

<textfield pref="true" prefattribute="value" prefstring="foobar.foo_bar" 
preftype="string" .../>

Not to mention that it suggests that no relationship exists between the 
attributes (but it does -- they're basically dependent on one another) since 
they're all top-level, independent attr.'s

But seeing this comma-delimited list in action is disappointing, also...I 
didn't realize it would look so confusing.  I'd still like to investigate other 
ideas, but that's not really within the scope of this bug.  Sorry for bringing 
it up, Eddy!

Comment 16

[eddyk]

Thanks for the feedback guys.

Alec, do you have any reservations about the %token within the prefstring? 
Right now, I feel that would be clearer and more flexible than pre/post-fix
attributes.  More than one token could be substituted, and it's easier to see at
a glance where the variable(s) is placed.

I agree on the separate attributes for type, value, etc.

Comment 17

[Alec Flett]

I guess I take issue with Yet Another Token Substitution method - there are a
million all over the codebase. I'm all for flexibility, but the way I designed
the account manager, no pref will ever have multiple keys in it... 

i.e. you'll never have mail.%server%.%identity%.foo or anything like that, and
in fact to do so would generate dependancies between the objects that don't
already exist, which would be bad.

if you must do token substituion, please at least do %varname% (double-%)
because that's what people are using (unfortunately) in the string bundles.

Comment 18

[eddyk]

Sorry to hoist YATS (tm) on the product.  At this point, I feel it's the best
solution.

It fits in better with the exiting pref usage, which I hope to integrate with
later.

LDAP and other pref users may need multiple substitution.

I wasn't aware of string bundles, and am looking at it for ideas and possible
areas where I might be able to share code/ideas.

Thanks again,

Comment 19

[eddyk]

Attachment: the next version, using prefstring attributes

Comment 20

[eddyk]

The 4/29 diff shows the implementation of locking using the same pref attributes
as used in the main pref dialog, with the exception of the %token% strings. 
We're not using WSM and nsPrefWindow yet, but this should simplify the transition.

This isn't complete yet, but I'd like to get some feedback if possible.  I still
need to lock the (new|delete|default) account buttons on the left side.  The new
account locking will require some fiddling with the main menu code.  It seems
like a good use for 'observes'.

I'm not sure how likely it is for full WSM and nsPref integration before the
final freeze.  If it turns out it can't make it in time, would a partial
implementation such as this be totally unacceptable?

Comment 21

[Alec Flett]

I'm warming to this approach after seeing the patch - it's at least consistent
with the nsPrefWindow stuff, as you said.

Comment 22

[eddyk]

First:  I've opened a new bug 79305 to handle the second portion of this task,
which is to use the same code as the general prefs dialog.

I'd like to check in this code, which can be summarised as the bare essentials
to get fe locking required by enterprise users.

This second patch I'll be adding locks the account button on the left side of
the AM dialog.  It alos disables the new account function available from the
account central page and from the File|New|Account menu item.

Comment 23

[eddyk]

Attachment: diff for account button and new account function locking

Comment 24

[eddyk]

I'd like to clarify why I'm looking to check this in ASAP.  5/11 is a code
freeze deadline I'd like to make.

Now I need review and sr.

Comment 25

[Alec Flett]

it seems like you should be looking at the locked status of the existing pref
"mail.accountmanager.accounts" to determine if add/delete should be enabled or
disabled...

Comment 26

[eddyk]

can I get sr= and r= on this patch + new file?

Comment 27

[eddyk]

Attachment: patches to m/n files using prefstrings (no wsm or prefwindow yet)

Comment 28

[eddyk]

Attachment: new file, am-prefs.js

Comment 29

[eddyk]

asked racham for r= and alecf for sr =

Comment 30

[Alec Flett]

patch looks ok, two requests:
1) add even more comments, this stuff is confusing
2) can you use nsIPrefBranch instead of the deprecated nsIPref?

Comment 31

[Alec Flett]

also, some of your indenting in the JS is wacky (the XUL looks fine) - it would
take more time for me to paste the places where it's wacky then it would for you
just look over the patch to see..

Comment 32

[eddyk]

Attachment: diffs with nsIPrefBranch and js indents cleaned up

Comment 33

[eddyk]

Attachment: am-prefs.js with additional comments

Comment 34

[racham]

r=racham.

Comment 35

[Marek Z. Jeziorek]

Setting target milestone to 0.9.2 (check it in anytime, even before, when the
tree is open for). Per PDT triage.

Comment 36

[Alec Flett]

cool thanks.. sr=alecf

Comment 37

[eddyk]

marking fixed.  for those interested, bug 79305 will be continuing this work.

Comment 38

[Rodney Velasco]

All mail/news account prefs are lockable using eddyk's test case to lock
prefstrings.  In order to lock the New Account Button you will need to specify
this lockPref function in the all-ns.js file or a hashed configuration file:
lockPref("mail.accountmanager.accounts", true);
Offline elements in the mail/news window is not lockable yet.  Filed separate
bug for that issue bug 82805.  I'm going to leave this as resolved fix until bug
82805 is resolved.  adding dianesun to Cc.

Comment 39

[Rodney Velasco]

Attachment: CFG to lock offline prefs

Comment 40

[Rodney Velasco]

Offline prefs section is now lockable, but there are other elements in Mail/News
account settings that need to honor locked prefs.  Still awaiting on bug 85335
before I can verify this bug.  I'll be filing more locking bugs since GUI has
changed since I filed this issue.  Will add the bugs as dependencies once filed.

Comment 41

[Rodney Velasco]

bug 85335 has been verified...this bug is too now verified due to dependency on
that bug.  Marking verified.