Closed Bug 949981 Opened 7 years ago Closed 7 years ago

collection of lockPref setting bugs (some structured options in preferences dialog are not disabled properly when the associated pref is locked)

Categories

(Thunderbird :: Preferences, defect)

24 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: spam2, Assigned: aceman)

References

(Blocks 1 open bug)

Details

(Keywords: conversion)

Attachments

(2 files, 3 obsolete files)

192.39 KB, video/avi
Details
4.54 KB, patch
aceman
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

Insert the following lines into mozilla.cfg
lockPref("mailnews.mark_message_read.auto", true);
lockPref("mailnews.mark_message_read.delay", false);


Actual results:

The corresponding radiogroup id="markAsReadAutoPreferences" is still enabled.
Only if you switch the radiobutton manually to the other option using the preferences gui the radiogroup gets disabled.


Expected results:

The corresponding radiogroup id="markAsReadAutoPreferences" should be grayed out and get disabled for user changes
Where should I put the mozilla.cfg file to try this out?
Assignee: nobody → acelists
At first you have to create a 'local-settings.js' file in the 'defaults\pref' subfolder of your thunderbird installation directory, e.g. '%ProgramFiles(x86)%\Mozilla Thunderbird\defaults\pref'
This 'local-settings.js' files must contain the following two lines:

pref("general.config.obscure_value", 0);
pref("general.config.filename", "mozilla.cfg");

After this you can create the 'mozilla.cfg' file in the Thunderbird installation directory, e.g. '%ProgramFiles(x86)%\Mozilla Thunderbird'

In the 'mozilla.cfg' file you have to put the following three lines:

//
lockPref("mailnews.mark_message_read.auto", true);
lockPref("mailnews.mark_message_read.delay", false);

It is important, that the first line in 'mozilla.cfg' file contains '//' as shown above.

Now you can run Thunderbird and try this out.
Attached patch patch (obsolete) — Splinter Review
Reporter, are you able to try out the patch?
Attachment #8347495 - Flags: review?(mkmelin+mozilla)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Unfortunately I am not able to compile Thunderbird on my own.
Is the attached diff belonging to source files in the thunderbird installation directory, that don't have to be compiled?
Yes, the change is in Javascript so you can find the file advanced.js in the omni.ja archive (a zip  archive), do the changes manually and repack the archive. Then delete the startupCache folder in your TB profile.
Thanks for the patch.
But I am not able to apply it.
It seems that the advanced.js you used for your changes is different from the one that resists in omni.ja archive for Thunderbird 24.2.0.
In my advanced.js file the function updateMarkAsReadOptions starts at line 300, whereas in your patch the same function starts at line 303.
That's a bit confusing.

Futher more it would be much easier for me if you could attach a complete advanced.js and advanced.xul, because I don't have any program installed to apply a diff to a source file automatically. I always have to use copy and paste :-( Sorry for that.
Meanwhile I did apply your proposed changes manually.
Good news: It's working now.
Bad news: There's one more bug left in this radiogroup

If you insert the following three lines in mozilla.cfg, only the textbox 'markAsReadDelay' should be disabled.

// lockPref("mailnews.mark_message_read.auto", true);
// lockPref("mailnews.mark_message_read.delay", false);
lockPref("mailnews.mark_message_read.delay.interval", 0);

But that's not the case. Since the other lines are commented out, the setting 'mailnews.mark_message_read.delay' defaults to 'false' or it is set to a value specified by the user before. If you switch the radio button manually in the GUI to 'true', the textbox 'markAsReadDelay' gets enabled again. If you klick on one of the spinbuttons next to this textbox, or if you manually change this value through typing, the textbox will be disabled again after having applied the change.
The right behaviour should be, that if you specify a lockPref setting for 'mailnews.mark_message_read.delay.interval', the corresponding textbox has to be always disabled, no matter if you switch the radiobuttons in the group 'markAsReadAutoPreferences'.
Perhaps we should rename the headline for this bug report:
'lockPref setting bugs'

Here's another one:

lockPref("mail.purge_threshhold_mb", 50);

Specifying this setting, the corresponding textbox 'offlineCompactFolderMin' should be disabled
(In reply to egal8888 from comment #7)
> If you insert the following three lines in mozilla.cfg, only the textbox
> 'markAsReadDelay' should be disabled.

Yes, I have noticed that too.
In the code you can see:
+    let intervalPref = document.getElementById("mailnews.mark_message_read.delay.interval");
     document.getElementById('markAsReadDelay').disabled =
+      !enableRadioGroup || !autoMarkAsPref.value || autoMarkAsPref.locked ||
+      intervalPref.locked;

So, I try to disable 'markAsReadDelay' if the "mailnews.mark_message_read.delay.interval" pref is locked. I do not understand why the textbox looks enabled but disables itself when clicked any input is attempted. But at least the user can't change the value.

May this be a bug of the widget?
Flags: needinfo?(neil)
(In reply to egal8888 from comment #8)
> Perhaps we should rename the headline for this bug report:
> 'lockPref setting bugs'
> 
> Here's another one:
> 
> lockPref("mail.purge_threshhold_mb", 50);
> 
> Specifying this setting, the corresponding textbox 'offlineCompactFolderMin'
> should be disabled

Yes, I think there are more fields where this checking is missing. If you are willing to test all of them then I can fix them.
(In reply to :aceman from comment #10)
> Yes, I think there are more fields where this checking is missing. If you
> are willing to test all of them then I can fix them.

At the moment I'm trying my best to roll out thunderbird to our company.
That's not as easy as I thought it would be.

So, please leave this bug opened until I set the status to "FIXED", since I have not tested all settings yet.
If I run into other bugs concerning lockPref settings, I will post them here, ok?
Summary: lockPref for mailnews.mark_message_read not working as expected → collection of lockPref setting bugs
(In reply to aceman from comment #9)
> So, I try to disable 'markAsReadDelay' if the
> "mailnews.mark_message_read.delay.interval" pref is locked. I do not
> understand why the textbox looks enabled but disables itself when clicked
> any input is attempted. But at least the user can't change the value.
> 
> May this be a bug of the widget?

Sorry, I'm unclear as to what the problem is here. Does the textbox disable correctly if you switch the radiobutton?
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk from comment #12)
> (In reply to aceman from comment #9)
> Sorry, I'm unclear as to what the problem is here. Does the textbox disable
> correctly if you switch the radiobutton?

No, it doesn't.
If you switch the radiobutton the textbox is enabled again.
Only in case a user tries to make a change to the value in this textbox, this box becomes disabled again.
Attached video DisableBug.avi
This video shows how Thundebird behaves when the radio button is switched.
(In reply to egal8888 from comment #11)
> So, please leave this bug opened until I set the status to "FIXED", since I
> have not tested all settings yet.
> If I run into other bugs concerning lockPref settings, I will post them
> here, ok?
Yes, we can try it. We can check in partial patches. If this proves to be too long (time-wise) we can close this bug and open a new one for new non-working fields.
(In reply to neil@parkwaycc.co.uk from comment #12)
> Sorry, I'm unclear as to what the problem is here. Does the textbox disable
> correctly if you switch the radiobutton?
The video shows the problem. We set .disabled on the textbox, however it does not appear disabled (it looks editable). As soon as you click the increment/decrement arrows or try to write into the textbox, it disables itself (without any code on our side).
Summary: collection of lockPref setting bugs → collection of lockPref setting bugs (some structured options in preferences dialog are not disabled properly when the associated pref is locked)
Oh, scratch it, false alarm. There actually is code attached to the preference that I didn't notice:
<preference id="mailnews.mark_message_read.delay"
                  name="mailnews.mark_message_read.delay" type="bool"
                  onchange="gAdvancedPane.updateMarkAsReadTextbox(this.value);"/>

 updateMarkAsReadTextbox: function(aFocusTextBox)
  {
    var textbox = document.getElementById('markAsReadDelay');
    textbox.disabled = !document.getElementById('markAsReadAfterDelay').selected;
    if (!textbox.disabled && aFocusTextBox)
        textbox.focus();
  },
Attached file advanced.js (obsolete) —
OK, this patch fixes the problem and also fixes the textbox from comment 8.
Can you try this full advanced.js ?
Attachment #8347495 - Attachment is obsolete: true
Attachment #8347495 - Flags: review?(mkmelin+mozilla)
Attachment #8348282 - Flags: feedback?(spam2)
Wow, thanks a lot.
All previous described bugs are fixed now.

But... the changes to advanced.js indroduce a new issue:

If you set lockPref("app.update.auto", false) the whole radiogroup 'updateMode' including checkbox 'warnIncompatible' has to be disabled.
With your patched advanced.js this is not the case. The checkbox 'warnIncompatible' stays enabled.
The original advanced.js from Thunderbird release 24.2.0 does not have this issue.

Could you please have a look at this?
Yeah, sorry, I attached the raw source of the file. It first needs to be preprocessed for each platform to remove unwanted code. Didn't you get errors in the Error console when running with this raw file? I do not think I have touched anything for update handling, therefore I think there may be parse errors so some code was not run properly.

For Windows, I think you need to remove all lines starting with #. NOT whole blocks inside of #ifdef, just the lines as the code inside the blocks is applicable on Windows.
(In reply to :aceman from comment #20)
> Didn't you get errors in the Error console when running with this raw file?

No, I did not get errors in the console.

> For Windows, I think you need to remove all lines starting with #. NOT whole
> blocks inside of #ifdef, just the lines as the code inside the blocks is
> applicable on Windows.

I removed all lines starting with # and yes, I can confirm that it is working now.

In which future version of Thunderbird will your patch be included?

I did not find any other issues with lockPref yet, but I have also not yet tested every option.
As this is my last week on work for this year :-) I think I will not finish testing in 2013.
(In reply to egal8888 from comment #21)
> In which future version of Thunderbird will your patch be included?
It should be in TB29, but if you ask about official stable releases then it is TB31, due out in about 30 weeks.

If you need it sooner for your enterprise, you can provide reasons why it is important and the release managers will decide whether to include it in a sooner maintenance release of TB24.x . If they see it is a low risk small patch.

I've linked this bug to our meta bug to track enterprise problems.

> I did not find any other issues with lockPref yet, but I have also not yet
> tested every option.
> As this is my last week on work for this year :-) I think I will not finish
> testing in 2013.
No, problem so we can then close this bug and if you find anything new in future you can open a new one.

But now before the big plans I need to attach the patch for review :)
Keywords: conversion
Attached patch patch (obsolete) — Splinter Review
Attachment #8348282 - Attachment is obsolete: true
Attachment #8348282 - Flags: feedback?(spam2)
Attachment #8350751 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8350751 [details] [diff] [review]
patch

Review of attachment 8350751 [details] [diff] [review]:
-----------------------------------------------------------------

r=mkmelin

::: mail/components/preferences/advanced.js
@@ +305,5 @@
> +   * state of the automatic marking feature.
> +   *
> +   * @param enableRadioGroup  Boolean value indicating whether the feature is enabled.
> +   */
> +  updateMarkAsReadOptions: function(aEnableRadioGroup)

@param aEnableRadioGroup
Attachment #8350751 - Flags: review?(mkmelin+mozilla) → review+
Attached patch patch v2Splinter Review
Thanks.
Attachment #8350751 - Attachment is obsolete: true
Attachment #8355623 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/25da80686360
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
You need to log in before you can comment on or make changes to this bug.