82805 - Offline and Disk Space Prefs should be lockable.

Status: VERIFIED FIXED

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

Description

[Rodney Velasco]

User Prefs 
"mail.server.server1.offline_download"
"mail.server.server1.download_bodies_on_get_new_mail"

need to be lockable according to eddyk's locking spec.  Information regarding
how to lock elements in Pref window can be found here...

news://news.mozilla.org/3AC8F5D2.6CA02623%40netscape.com

There are no prefstrings available for select box and Disk Space pref elements,
those elements will need to be locked as well.

Test Case:

1. Open your all-ns.js (or all.js if using mozilla) file and add the following
lines to that file
   lockPref("mail.server.server1.offline_download", true);
   lockPref("mail.server.server1.download_bodies_on_get_new_mail", true);
2. Start Mozilla Mail Client and create an account if you haven't already
3. Open Mail/News Account Prefs window Edit -> Mail/News account settings and
   the Offline and DiskSpace field.
4. All Offline check boxes and buttons should be greyed out and checked.

Again Disk Space area does not have any prefstrings.  Once prefstrings have been
added they must be lockable as well.

Comment 1

[Rodney Velasco]

assigning QA to myself, adding esther to Cc.

Comment 2

[Rodney Velasco]

adding jpm to cc.

Comment 3

[Rodney Velasco]

this blocks bug 70623 from being verified. Need some traction on this please.

Comment 4

[Diane Sun]

working on it.

Comment 5

[Diane Sun]

This is actually preference issue

Comment 6

[Diane Sun]

Attachment: Fix to lock the preference

Comment 7

[Diane Sun]

Need clarification regarding 

User Prefs 
"mail.server.server1.offline_download"
"mail.server.server1.download_bodies_on_get_new_mail"

are actually for which checkbox. 

Comment 8

[Rodney Velasco]

These are preferences for the offline section.
User Prefs 
"mail.server.server1.offline_download" = When I create new folders, select them
for offline

"mail.server.server1.download_bodies_on_get_new_mail" = Make the messages in my
Inbox available when I am working offline

Comment 9

[Diane Sun]

Attachment: Modified fix

Comment 10

[eddyk]

Why does locking a pref also cause it to be checked?  Can't it be locked to an 
unchecked state?  Rodney, correct me if I'm wrong, but I don't think you 
really meant this:
"4. All Offline check boxes and buttons should be greyed out and checked."

What about the "Select.." button that brings up the folders dialog?  I suggest 
using a new pref.  This has been done for many other buttons.  Generally I've 
used the format: "whereever_your_section_belongs_to.disable_button.button_name"
e.g  "mail.disable_button.set_default_account"  And then set the disabled 
attribute on the button if the pref is locked.  Let me know if you need 
clarification on this.

The Disk Space section is not being addressed by this patch, and I believe it 
should be.

Comment 11

[Rodney Velasco]

For the specified testcase I described the boxes should be checked because the
lockPref value is set to true for both cases...

 lockPref("mail.server.server1.offline_download", true);
 lockPref("mail.server.server1.download_bodies_on_get_new_mail", true);

In a general case of course the values won't be checked if the preference is set
to false, I  was just trying to descibe what the user would see in the offline
area if they used my testcase.  As for the Select button, yes that too does need
to be addressed, I described it my initial description as a select box...maybe I
should have been more descriptive and called it a "Select Button".

Comment 12

[Rodney Velasco]

to clarify again I meant "Select.." button not "Select Button"

Comment 13

[Rodney Velasco]

Revised Test Case:
Before you begin, delete your prefs.js file
Linux: "~/.mozilla/your_user_name/salted directory/prefs.js"
Mac: "Hard Drive: Documents : Mozilla : Profiles : Username : salted directory :
prefs.js"
WinNT: "C:\WinNT\Profiles\your_user_profile\Application
Data\Mozilla\Profiles\Username\salted directory\prefs.js"

1. Open your all-ns.js (or all.js if using mozilla) file using any file editor
and add the following
lines to that file
   lockPref("mail.server.server1.offline_download", true);
   lockPref("mail.server.server1.download_bodies_on_get_new_mail", true);
2. Start Mozilla Mail Client and create an account
3. Open Mail/News Account Prefs window Edit -> Mail/News account settings and
   the Offline and DiskSpace field.

Expected Result:
All "Offline Section" check boxes should be greyed out and checked (not checked
if you set the lockPref value to false).

Actual Result: 
Boxes are checked but not locked.

Comment 14

[racham]

Patch looks ok to me except (as eddyk pointed out)

+        document.getElementById("offline.newFolder").checked = true; 
+        document.getElementById("offline.downloadBodiesOnGetNewMail").checked =
true;

We shouldn't be explicitely setting these values. They are what prefs say they
are and 

*     document.getElementById("offline.downloadBodiesOnGetNewMail").checked = 
gImapIncomingServer.downloadBodiesOnGetNewMail;
*     document.getElementById("offline.newFolder").checked = 
gImapIncomingServer.offlineDownload;

will take care of them.

Rodney's test case should succeed without us having to set checked attribute
explicitely to true as the pref value will be grabbed anyway. Infact having
checked to true will simply force that behavior no matter what the pref value is.

Let me know if there is any other purpose behind it.

bhuvan

Comment 15

[Diane Sun]

Attachment: Removed the checked lines

Comment 16

[racham]

r=bhuvan.

Please test the lock feature before you checkin. thanks.

bhuvan

Comment 17

[eddyk]

what about the disk-space ui elements?  It doesn't look like they're being 
disabled.

Comment 18

[Diane Sun]

disk-space ui elements  are  not related to Preferences 
"mail.server.server1.offline_download"
"mail.server.server1.download_bodies_on_get_new_mail"

Comment 19

[Rodney Velasco]

and how bout the select button? is that lockable too? If so, what is the
prefstring for that button?

Comment 20

[eddyk]

The initial bug report states:

There are no prefstrings available for select box and Disk Space pref elements,
those elements will need to be locked as well.

Comment 21

[Diane Sun]

In the account manager,  the preference is per-server based, not global, there 
is no prefstring associated with them.
If there is any other preference should be locked based on Lock_pref,  please 
file bugs. 

Please also confirm if "mail.server.server1.offline_download"  is set in the 
lock preference,   the "Select" button should be locked.

Comment 22

[Rodney Velasco]

filed bug 85335 for disk space UI issue.

Comment 23

[Jussi-Pekka Mantere]

PDT+ per 6/12 mtg.

Comment 24

[Diane Sun]

Request SR again. 

Comment 25

[(not reading, please use [e-mail] instead)]

aren't you supposed to be using the pref branch to lock prefs, instead of pref 
service directly?

see am-prefs.js, getAccountValueIsLocked()

eddy k / bness might know more about this.

Comment 26

[Diane Sun]

It would be nice to use getAccountValueIsLocked().  But 
getAccountValueIsLocked() requires to use "prefstr".  "prefstr" is for general 
preference, ie global preference usage.
For the account manager panel,  every preference is per server based. Each 
preference lock has to be processed indivisually. 

Comment 27

[eddyk]

Every other panel using prefstr attributes is per-server or per-identity based.

In AccountManager, the prefstring is not currently handled by nsPrefWindow.  It
is using its own code where the prefstring can be per-server based.

This is done by using tokens and and allowing the prefstring handling code to
substitute the server value in the prefstring.

If you look at other panels, you'll see the tokens %serverkey% and %identitykey%
among others.

These get filled in by automatically by the token substitution code.  You should
be able lock the new xul elements if you know the prefstring.

Soon, the custom code in AM will be replaced with an updated nsPrefWindow which
should handle all this.



 

Comment 28

[Brian Nesse (gone)]

Yes, Seth is correct. Please do not add new code which uses nsIPref, use 
nsIPrefService & nsIPrefBranch instead. A rough example would look like:

var prefService = Components.classes["@mozilla.org/preferences-service;1"];
prefService = prefService.getService();
prefService = prefService.QueryInterface(Components.interfaces.nsIPrefService);

finalPrefString = initPrefString + "." + gIncomingServer.key + "."
var pref = prefService.getBranch(finalPrefString);
isDownloadLocked = pref.PrefIsLocked("offline_download");
     .    .
     .    .
     .    .
isGetNewLocked = pref.PrefIsLocked("download_bodies_on_get_new_mail");

Comment 29

[Diane Sun]

Attachment: latest fix using pref branch

Comment 30

[Brian Nesse (gone)]

You only have to get the branch once. You don't need to get it for each
preference call.

Comment 31

[Diane Sun]

Attachment: get rid of one call for branch

Comment 32

[eddyk]

In your xul file, you can disable buttons using the following attributes in the
button element.

pref="true" preftype="bool" prefattribute="disabled"
prefstring="mail.offline.disable_button.select_folder"

or if you want to make it per-server based locking, add in a %serverkey% token.

Comment 33

[Diane Sun]

Attachment: Create prefstring in *.xul file for using pref attribute

Comment 34

[eddyk]

You have some typos:

preftrype   -> preftype

Comment 35

[Diane Sun]

Attachment: New xul prefrence

Comment 36

[eddyk]

Sorry, a couple more things.

1) the prefattribute attribute for checkboxes should be "checked" not disabled.

 prefattribute="checked"

Only buttons should be set disabled for that attribute.


2) The radiogroup (id=nntp.keepMsg) does not have a complete set of attributes.
 It has pref, prefattribute, and preftype but it doesn't have prefstring.  It
should have all 4 or none.

While there may be other issues, they probably can't be addressed until I finish
bug 79305

r=eddyk on the xul changes.

Comment 37

[Diane Sun]

Attachment: Change to "checked" for checkbox

Comment 38

[racham]

Looks good. r=bhuvan.

Comment 39

[(not reading, please use [e-mail] instead)]

I'm totally confused as to what the complete patch is.

is the complete patch just the last attachment?

for my own benefit, do you test lock prefs?  where do you set them as locked? 

Comment 40

[(not reading, please use [e-mail] instead)]

s/do you test lock prefs?/How do you test locked prefs?

Comment 41

[Diane Sun]

The complete patch includes attachment  38648 [details] [diff] [review] and 38993.
The 38648 is actually locking the preference using the xpcom, AM approach,
the 38993 is for future nsIPref us to make them lockable generically.
I'll make a complete patch.

To test the locked preference,  you still use the same lock pref in all.js.

To actually test the lock by using prefstring only,  we have to wait Eddy 
checkin bug 79305. He actually tested the xul change in his own tree.

Comment 42

[Diane Sun]

Attachment: Complete patch for js and xul changes

Comment 43

[(not reading, please use [e-mail] instead)]

wait, so the xul of your patch doesn't do anything until eddyk checks in?
and when eddyk checks in, you'll be removing the js part of your fix?

if that is the case, don't do that.

just attach a patch for the current fix, and open a new bug (dependent on 
eddyk's fix) to remove the .js and make the .xul changes.

Comment 44

[Diane Sun]

OK,   Could we approve the js change only this time,  then move the xul change 
to another bug.

Comment 45

[(not reading, please use [e-mail] instead)]

what's the final fix?  please re-attach a final fix.

Comment 46

[Diane Sun]

Attachment: The complete js changes to fix the bug through xpcom approach

Comment 47

[racham]

Diane,

Please do file a new bug on adding your XUL changes & removing the js changes 
contingent to eddyk's changes before checking in so that we have a track for the 
actual bug/fix.

r=bhuvan.

Comment 48

[(not reading, please use [e-mail] instead)]

sr=sspitzer

Comment 49

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 50

[Diane Sun]

Fix checked in.

Comment 51

[Rodney Velasco]

Reopening bug.  Cannot lock preferences using 2001062008 builds.  Tried locking
preference in all-ns.js and using a netscape.cfg file.  This also caused a
regression with offline prefs.  Gary Chan is filing bug for this issue.

Added this pref in all-ns.js file:
lockPref("mail.server.server1.offline_download", true);
lockPref("mail.server.server1.download_bodies_on_get_new_mail", true);

Started commercial build, and preferences were checked....but not locked.

Comment 52

[(not reading, please use [e-mail] instead)]

PrefIsLocked should have been prefIsLocked.

see bug #86996

Comment 53

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to frozen 0.9.2.
(on behalf of drivers)

Comment 54

[(not reading, please use [e-mail] instead)]

fixed.  I'm able to lock these prefs now.

Comment 55

[Rodney Velasco]

Attachment: CFG to lockdown offline prefs

Comment 56

[Rodney Velasco]

Using the CFG I provide above I was able to lockdown the offline prefs:
"mail.server.server1.offline_download"
"mail.server.server1.download_bodies_on_get_new_mail"

Tested on 
2001062508 Linux RH7.0
2001062303 Mac OS 9.1 (place CFG in mozilla install directory: Essential Files dir)
2001062509 Win2000

Select Button is not lockable, will file new bug to get that preference element
in Mail/News prefs to lock.

Comment 57

[Diane Sun]

Select Button will be handled by bug 86778.