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

Status: VERIFIED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Matthew (lilmatt) Willis]

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.

Comment 1

[Joey Minta]

Attachment: set border to none

When we catch the error for an empty preference, simply returning leaves the old rule.  This patch makes sure we reset the border style.

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 3

[Joey Minta]

Attachment: delete rule

Now we actually find the rule, and in the case that the pref doesn't exist, we delete the rule.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 5

[Joey Minta]

(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?

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

(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 7

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 227754 [details] [diff] [review]
delete rule

minussing, to get it out of my queue.

Comment 8

[Joey Minta]

Attachment: still delete rule, and fix getPrefSafe

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.

Comment 9

[Matthew (lilmatt) Willis]

Comment on attachment 231518 [details] [diff] [review]
still delete rule, and fix getPrefSafe

r=lilmatt

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 11

[Joey Minta]

Patch checked in.

Comment 12

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060902 Calendar/0.3a2+

Comment 13

[Damian Szczepanik]

Litmus testcase 2625 created