85335 - Disk Space UI Elements need to be lockable

Status: VERIFIED FIXED

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

Description

[Rodney Velasco]

Per dianesuns request on bug 82805, filing bug for disk space UI elements need
to be lockable.  The prefstrings for these elements in 4.x were

"mail.max_size" = Do not download any message larger than: XX K (XX denotes number)

"mail.purge_threshhold" = Compact folders when it will save over: XX K

Comment 1

[Rodney Velasco]

QA over to me

Comment 2

[Rodney Velasco]

adding gary chan to Cc.

Comment 3

[Diane Sun]

Is "mail.max_size" a global preference or  a  per-server one?

Comment 4

[Rodney Velasco]

by looking at the UI "mail.max_size" appears to be a per server type of pref.

Comment 5

[Diane Sun]

In that case, in 6.x,  should that be "mail.server.severxx.max_size"?

Comment 6

[Diane Sun]

In 4.x there is  Disk Space preference  in Mailnews&Newsgroups.
Some preferences for newsgroup are not secified or request here.  Please list 
all your reqest preferences.  Thanks.

Comment 7

[Diane Sun]

Actually,  mail.purge_threshhold is for the text field, and 
mail.prompt_purge_threshold is for the checkbox.  In my previous bug fix,  I 
added the prefstring the these preferences.  But  the text field needs to be 
locked when the the checkbox is unchecked, the same requirement as 87177.

the mail.max_size is also the text field which is already locked by the checkbox 
in 87177 but we need to add further prefstring for the 
checkbox/radio preferences in Disk Space. 

Comment 8

[Jussi-Pekka Mantere]

Reassigning.

Comment 9

[eddyk]

I'm working on a patch that should close this and bug 86778.

The prefstrings that are used to lock the elements are:

mail.server.[serverkey].limit_message_size
mail.server.[serverkey].max_size
mail.server.[serverkey].downloadUnreadOnly
mail.server.[serverkey].downloadByDate
mail.server.[serverkey].ageLimit
mail.server.[serverkey].retainBy
mail.server.[serverkey].daysToKeepHdrs
mail.server.[serverkey].numHdrsToKeep
mail.server.[serverkey].keepUnreadOnly
mail.server.[serverkey].daysToKeepBodies
mail.server.[serverkey].disable_button.selectFolder

Comment 10

[eddyk]

Attachment: patch to lock panel elements

Comment 11

[eddyk]

Description of patch:

.js changes - do a lock/unlock check on page init.  Pretty straightforward

.xul changes - This is from bug 86014.  It removes some code and changes some
existing stuff.  Apparently someone added (or moved) code into the panel without
checking the existing ids.  There was a conflict with 2 identical ids causing
the element to stop working properly.

Comment 12

[eddyk]

just talked to srilatha and got an r=

Comment 13

[Scott MacGregor]

can you get an r= from the module owner (Bhuvan) first? Once he's looked at it,
i'll super review it.

Comment 14

[racham]

Eddy,

patch looks OK. I think we can go for couple of minor improvements here.

1. "var pref = prefService.getBranch(finalPrefString);" , we should proabbly
make pref a global (gPref or a more context sensitive name). That way we can
skip passing that var all the time to disableIfLocked().

2. disableIfLocked() is called several times. Instead, we can just call it once
 and pass it an array of prefstring and id pairs. Then , we can simply loop in
disableIfLocked. We should add comments on the top that array indicating any new
items to be added to the list of locakable pref should be added to that list.

3. I noticed some changes in xul file that are strictly not related to locking
prefs (correct me if I am wrong). If you need to get them in, please post
screenshots for POP, IMAP and News panels as a part our regular UI review change
approvals.

thanks
bhuvan

Comment 15

[eddyk]

- Did the suggested changes.  Thanks for the excellent feedback.
- You are correct, the xul file is not strictly for locking.  I found that one
of the checkboxes was broken and that diane's change was still not checked in. 
I thought I would incorporate her change and get it checked in as well.  The
existing bug and screenshots are in bug 86014.  The only changes I made were to
update from the old xul syntax for box to the new hbox.

Would you prefer that I leave that part out and let the original bug stand? 
(She is aware of my change and intent to check it in)

Comment 16

[racham]

Eddy,

Did you forget to post the updated patch here..? Please post the new one so that
you can go the next step i.e., the super review.

With respect to the XUL file changes, I think checkins should happen from
corresponding bugs with r & sr= done in both bugs. I noticed that the other bug
86014 has already been reviewed. Just get sr there and you(/diane) should be
good to go. Sorry for the extra work. But, I think it will be a better way to go
about it (also I noticed that super reviewers are different bug these 2 bugs, so
it is better to stick to the domain the bug to make reviews go faster).

Comment 17

[eddyk]

Attachment: patch with only .js changes, no xul changes

Comment 18

[racham]

May you want to get document.getElementById(prefstrArray[i].id) in this into a
local var like offlineElement or something (in routine disableIfLocked) and then
use offlineElement.disabled() & offlineElement.removeAttribute().

r=bhuvan

Comment 19

[eddyk]

Attachment: patch to .js with final comment by bhuvan incorporated.

Comment 20

[Scott MacGregor]

everything looks good.

sr-mscott

Comment 21

[eddyk]

set milestone to 0.9.4

Comment 22

[eddyk]

checked-in fix.

Comment 23

[Diane Sun]

I have commented onLockPreference() out in am-offline.js since it causes 
regression.  Please rework on is after 86014 is checked in.
By the way, could you move the global var gPref to the top?

Comment 24

[eddyk]

re-opening

Comment 25

[eddyk]

Attachment: new patch to better integrate with existing code.

Comment 26

[eddyk]

Description of changes:
  * moved gPref to top of file.
  * added global associative array to keep track of prefs -> locked state.
  * added checks in existing logic that disables/enable elements to also check
the global associative array to avoid removing the disabled attribute when
appropriate.
  * added some code comments

Since Bhuvan is on vacation still, can I get someone else to review this patch?
 Diane?

Comment 27

[Diane Sun]

r=dianesun on 45237

Comment 28

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

sr=sspitzer

Comment 29

[eddyk]

check-in

Comment 30

[Rodney Velasco]

All prefs lockable except for the checkbox

"mail.server.[serverkey].daysToKeepBodies"

This is part of the mail/news Offline and Disk Space section.  Eddy, I believe
this is locked in the backend because the value does not change if I uncheck the
box or change the value.

I also had another question as to how to enable a radio box for the pref....

"mail.server.[serverkey].retainBy"

Through my testing I found that this preference controls the radio box disabling
for all elements listed in the radio box selection.  The default value for this
pref is an int = 1. By default this chooses the "Keep all messages" radio box
option in the disk space section.  What if I wanted the "keep messages which
have arrived with the past XX days" radio box to be pressed and locked down.  I
could lockdown the XX days by specifying this pref
"mail.server.server4.daysToKeepHdrs", but this doesn't set the radio box value.

Comment 31

[grylchan]

The UI bug for the pref 'remove message bodies older than x days'
is bug 91560.

Comment 32

[Rodney Velasco]

Cancel my last comments...figured out that the radio buttons for the news
offline prefs are working, but in an unorthodox way.


lockPref("mail.server.server3.retainBy", 2); /* This pref refers to the radio
button..."Keep messages which have arrived with the past", this does not however
control the value of the textbox */

lockPref("mail.server.server3.retainBy", 1); /* This pref refers to the radio
button..."keep all messages"...which is the actually the middle radiobox element */

lockPref("mail.server.server3.retainBy", 3); /* This pref refers to the radio
button "Keep the Newest XX messages"...however this does not refer to the
numrical value for XX...thats controlled by another pref */

Marking this verified.

Verified on branch bits:
Linux: 20010927
Win2K: 20010927
MacOS 9.1: 20010926MacOS X: 20010927