Closed Bug 1004685 Opened 5 years ago Closed 5 years ago

rename all occurences of "check" attribute in mailWindowOverlay.xul to "checked"

Categories

(Thunderbird :: Mail Window Front End, defect, trivial)

defect
Not set
trivial

Tracking

(thunderbird31 fixed, thunderbird32 fixed)

RESOLVED FIXED
Thunderbird 32.0
Tracking Status
thunderbird31 --- fixed
thunderbird32 --- fixed

People

(Reporter: aceman, Assigned: rsx11m.pub)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

I think the usage of "check" attribute is a typo. The attribute should be "checked" as can be seen when searching on how the elements containing it are used. Just searching for the IDs shows the code touching those elements sets the "checked" attribute.

The attribute setting in XUL file probably just marks the initial value that is overwritten by code soon enough so this typo maybe didn't cause any problems.
It probably works despite the type because the "checked" attribute should default to "false" if not set. Thus, it may be equally plausible to just remove the three check="false" lines to fix this.
s/type/typo/ ... ;-)
This works for me in all three instances of the "Favorite Folder" menuitem ...
Assignee: syshagarwal → rsx11m.pub
Attachment #8416581 - Flags: review?(squibblyflabbetydoo)
Attached patch Patch removing the attribute (obsolete) — Splinter Review
... and this works as well, thus it may mainly be a question of whether or not to explicitly document the default setting of these menuitems.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/checked recommends however to "Use hasAttribute() to determine whether this attribute is set instead of getAttribute()." In this case, removing the attribute entirely to represent a value "false" seems to be the more correct way to go.

Please pick and (hopefully) r+ one of these patches, whichever you prefer.
Attachment #8416584 - Flags: review?(squibblyflabbetydoo)
rsx11m, in that case you would need to also fix the code that sets the checked state of those elements. E.g. at http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#126 it does set "checked=false". And also other code consuming the attribute of those elements (if any). So you can't operate the attribute in 2 different interpretations. If you do not fix that code, then checked=false as the default would be more correct.

Also, please do not hijack recently assigned bugs :)
Attachment #8416581 - Attachment is obsolete: true
Attachment #8416581 - Flags: review?(squibblyflabbetydoo)
Attachment #8416584 - Attachment is obsolete: true
Attachment #8416584 - Flags: review?(squibblyflabbetydoo)
Ok, sorry about that. I saw that you initially assigned it to Suyash, but as he is afk right now and it looked like an easy fix (which apparently it is as long the attribute isn't removed), I simply posted the patch. Thus, returning it onto his plate.
Assignee: rsx11m.pub → syshagarwal
I see Seamonkey never has checked=false in any xul file. TB has some more occurrences than these *_favoriteFolder ones. I just say that just removing them is risky and needs a code audit whether all consumers of those elements properly use hasAttribute().

So of course, we'd like to hear opinion from squib and mkmelin on which variant we should follow.
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(mkmelin+mozilla)
I agree it looks like a typo kind of thing, and should just be removed.
We can probably leave the (real) checked="false" alone if nothing is broken - as I recall iẗ́'s really hard to get it right (the checked *property* get sets only on if the element is visible or something like that)
Flags: needinfo?(mkmelin+mozilla)
(In reply to rsx11m from comment #6)
> Ok, sorry about that. I saw that you initially assigned it to Suyash, but as
> he is afk right now and it looked like an easy fix (which apparently it is
> as long the attribute isn't removed), I simply posted the patch. Thus,
> returning it onto his plate.

Hi, please fix it; in fact you you already have done so why do you want me to go through this again when you already have a patch :)
Please bring up the patch so that we can end this bug really quickly.

Thanks.
(In reply to Suyash Agarwal (:sshagarwal) away till 7th of May from comment #9) 
> Hi, please fix it; in fact you you already have done so why do you want me
> to go through this again when you already have a patch :)
> Please bring up the patch so that we can end this bug really quickly.

Ok, it's not clear to me if there is any special deal involved that aceman assigns bugs directly to you = don't want to steal your jobs...  :-)

(In reply to Magnus Melin from comment #8)
> I agree it looks like a typo kind of thing, and should just be removed.

Ok, so this would suggest that you prefer the second version removing the attribute for good in these three instances, but is any further investigation needed as was suggested by aceman?

> We can probably leave the (real) checked="false" alone if nothing is broken
> - as I recall iẗ́'s really hard to get it right (the checked *property* get
> sets only on if the element is visible or something like that)

But then, if I read the second part correctly, "leave alone" would imply that you just want me to correct the typo but other than that not touching it any further? (i.e., the first patch as is?)
Assignee: syshagarwal → rsx11m.pub
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
(In reply to rsx11m from comment #10)
> Ok, it's not clear to me if there is any special deal involved that aceman
> assigns bugs directly to you = don't want to steal your jobs...  :-)
I wouldn't assign a bug to anybody if I hadn't discussed with him previously that he wants to do it.
So if you make a patch in parallel to him, it may be wasted work on one side.

> (In reply to Magnus Melin from comment #8)
> > I agree it looks like a typo kind of thing, and should just be removed.
> 
> Ok, so this would suggest that you prefer the second version removing the
> attribute for good in these three instances, but is any further
> investigation needed as was suggested by aceman?
As the code sets checked=false, it probably works with it fine. So if we keep checked=false in the xul file
it should be safer than removing it.

> > We can probably leave the (real) checked="false" alone if nothing is broken
> > - as I recall iẗ́'s really hard to get it right (the checked *property* get
> > sets only on if the element is visible or something like that)
> 
> But then, if I read the second part correctly, "leave alone" would imply
> that you just want me to correct the typo but other than that not touching
> it any further? (i.e., the first patch as is?)
It looks like he meant not touching the code that sets checked=false.
There is actually a ton of code that just sets .setAttribute("checked", some_bool_variable). So fixing all
that would be harder.
Comment on attachment 8416581 [details] [diff] [review]
Patch correcting the typo

Ok, so let's go with this one unless squib disagrees.
Attachment #8416581 - Attachment is obsolete: false
Attachment #8416581 - Flags: review?(squibblyflabbetydoo)
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(mkmelin+mozilla)
(In reply to :aceman from comment #11)
> As the code sets checked=false, it probably works with it fine. So if we
> keep checked=false in the xul file
> it should be safer than removing it.

The safest thing to do is to just remove it, as the xul does NOT currently have checked=false, it has check="false" which is nothing.
Attachment #8416581 - Flags: review?(squibblyflabbetydoo) → review+
I have double-checked the three occurrences where the favorite-folder checkbox status is set, and it's using .setAttribute("checked", folders[0].getFlag(kFavoriteFlag)); in all cases. In the backend, a simple folder.toggleFlag(Components.interfaces.nsMsgFolderFlags.Favorite); is used on changing the checkbox status, apparently not reading the checkbox status back but just updating its status when the menu becomes visible. Thus, checked="false" should be the correct initialization for that usage.

Thanks for the review and push for comm-central, please.
Keywords: checkin-needed
Blocks: 1008506
I've opened bug 1008506 on the general issue of whether or not to replace the true/false representation with present/non-present ones as suggested by the documentation.

Also, for the record, the typo fixed here was initially introduced in bug 251296 for two of the instances and later by bug 650170 for the application button (obvious copy-pasting).
https://hg.mozilla.org/comm-central/rev/827ecdb1b55e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 32.0
Comment on attachment 8416581 [details] [diff] [review]
Patch correcting the typo

[Approval Request Comment]
Regression caused by (bug #): typo introduced by bug 251296 and bug 650170
User impact if declined: none
Testing completed (on c-c, etc.): tested in a 31.0 build
Risk to taking this patch (and alternatives if risky): low

While the impact seems to be of rather cosmetic nature without actually impairing function, it would be nice to get this typo corrected in 31.0 as well to have it fixed in the upcoming release branch.
Attachment #8416581 - Flags: approval-comm-aurora?
Attachment #8416581 - Flags: approval-comm-aurora? → approval-comm-aurora+
Keywords: checkin-needed
Whiteboard: [c-n: comm-aurora]
You need to log in before you can comment on or make changes to this bug.