Closed Bug 340229 Opened 14 years ago Closed 14 years ago

Removing a color completely from a category doesn't update the rule

Categories

(Calendar :: Sunbird Only, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mattwillis, Assigned: jminta)

Details

Attachments

(1 file, 2 obsolete files)

In testing bug 333923 jminta noticed that removing a color completely from a category doesn't update the rule. It doesn't look to be a regression.
Attached patch set border to none (obsolete) — Splinter Review
When we catch the error for an empty preference, simply returning leaves the old rule.  This patch makes sure we reset the border style.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
Attachment #225796 - Flags: first-review?(mvl)
Comment on attachment 225796 [details] [diff] [review]
set border to none

Setting the border style to 'none' is wrong. Themes might give items a border. (maybe the default theme even does, i'm not sure)
We somehow need to reset or remove the rule.
Attachment #225796 - Flags: first-review?(mvl) → first-review-
Attached patch delete rule (obsolete) — Splinter Review
Now we actually find the rule, and in the case that the pref doesn't exist, we delete the rule.
Attachment #225796 - Attachment is obsolete: true
Attachment #227754 - Flags: first-review?(mvl)
Comment on attachment 227754 [details] [diff] [review]
delete rule

>Index: resources/content/calendarManagement.js
>+    var ruleIndex;
>     function getRuleForObject(name)
>     {
>+                ruleIndex = i;

My c++ side doesn't like the use of a global varialbe as a return parameter (or in this case, some var in a bigger scope). I think we should look for a different way to return the value.
(In reply to comment #4)
> (From update of attachment 227754 [details] [diff] [review] [edit])
> My c++ side doesn't like the use of a global varialbe as a return parameter (or
> in this case, some var in a bigger scope). I think we should look for a
> different way to return the value.
> 
Would 
  function getRuleDataForObject(aName) { 
    ...
     return {index: i, rule: rule};
  }
be an acceptable solution for you or do i need to do deeper refactoring?
(In reply to comment #5)
> Would 
>   function getRuleDataForObject(aName) { 
>     ...
>      return {index: i, rule: rule};
>   }
> be an acceptable solution for you or do i need to do deeper refactoring?
> 

Yeah, that will do. JS makes that easy, so let's just use it. (And it agrees with my old perl background :) )
Comment on attachment 227754 [details] [diff] [review]
delete rule

minussing, to get it out of my queue.
Attachment #227754 - Flags: first-review?(mvl) → first-review-
This patch still deletes the rule, but the function in question how now changed significantly.  rule and ruleIndex are now in the right scope, so we don't need to go extra stuff with assignment/returning.  Testing this bug, however, identified a problem in getPrefSafe.  If you call deleteBranch, it will still return a valid value for the type of that pref.  So, we need to put the get*Pref() call into a try block anyway.
Attachment #227754 - Attachment is obsolete: true
Attachment #231518 - Flags: second-review?(mvl)
Attachment #231518 - Flags: first-review?(mattwillis)
Comment on attachment 231518 [details] [diff] [review]
still delete rule, and fix getPrefSafe

r=lilmatt
Attachment #231518 - Flags: first-review?(mattwillis) → first-review+
Comment on attachment 231518 [details] [diff] [review]
still delete rule, and fix getPrefSafe

> function getPrefSafe(aPrefName, aDefault) {
>+    var getFunc;
>     switch (prefB.getPrefType(aPrefName)) {
>         case nsIPrefBranch.PREF_BOOL:
>-            return prefB.getBoolPref(aPrefName);
>+            getFunc = prefB.getBoolPref;
>+            break;
>+    try {
>+        return getFunc(aPrefName);

Nice use of javascript features, but for readability, just put the entire swtich block in the try block, and don't mess with a getFunc.

r=mvl with that changed.
Attachment #231518 - Flags: second-review?(mvl) → second-review+
Patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060902 Calendar/0.3a2+
Status: RESOLVED → VERIFIED
Whiteboard: [litmus testcase wanted]
Litmus testcase 2625 created
Whiteboard: [litmus testcase wanted]
You need to log in before you can comment on or make changes to this bug.