396223 - Only set calendar preferences if they have changed

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: Fix v1

As an optimization especially for async providers that watch calendar prefs, there should be no notification if the calendar preferences are not changed while calling setCalendarPref

Comment 1

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

I'm not sure if this pach is correct. You can longer be sure of the observer will be called. So you don't have any 'feedback' on setting a pref (if you need it).

If there are observers that need to know if the value really changed, we can add a |in boolean aChanged| parameter to onCalendarPrefSet. Another way would be to add a method onCalendarPrefChanged.

Comment 2

[Daniel Boelzle [:dbo]]

(In reply to comment #1)
Right, but to me this sounds like that code (mvl is mentioning) already lacks an explicit notification mimic; relying on onCalendarPrefSet sounds rather fragile to me.

Comment 3

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

After having a bit more sleep, i'm not sure of this would really be a problem. But I still think it would be cleaner if we notify always, and add information about whether the pref really changed.

Comment 4

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: Better Fix - v1

As discussed, this is a better approach. The old value is passed to the listener so that it can decide if it should act itself.

Comment 5

[Daniel Boelzle [:dbo]]

Sorry, but I am not convinced by this change; IMO the stated optimization is sensible. I'd prefer that we find and change the code that relies on those side effects, e.g. code that resets a property only to trigger a view refresh.

Comment 6

[Philipp Kewisch [:Fallen][☁️📆]]

To make things clear, what about getting rid of onCalendarPrefSet altogether? It could be replaced by onCalendarPrefChanging or onCalendarPrefChanged.

Comment 7

[Philipp Kewisch [:Fallen][☁️📆]]

I know I am pretty late with this, but I would really like to have either solution in 0.7. It would really simplify things for gdata. I am attaching another solution that replaces onCalendarPrefSet with onCalendarPrefChanged.

Comment 8

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: onCalendarPrefChanged approach - v1

Comment 9

[Philipp Kewisch [:Fallen][☁️📆]]

Note I missed two files that have onCalendarPrefSet in most patches. They only have an empty function though, so its only renaming the functions. I'll do that at latest before checkin.

Note also, that the only code that actually does something in onCalendarPrefSet is the calendar-management.js. There, its fine to only notify if the value changed. I think if we change the semantics this early and document correctly, then its fine to change it.


http://mxr.mozilla.org/seamonkey/search?string=onCalendarPrefSet
http://mxr.mozilla.org/seamonkey/search?string=setCalendarPref

Comment 10

[Simon Paquet [:sipaq]]

I'm not in the position to grant or to deny this approval, but what I really would like to see, before anyone makes a decision here, is a short risk analysis.

What is the regression risk? Which areas should we look at when looking for potential regressions?

Comment 11

[Philipp Kewisch [:Fallen][☁️📆]]

Looking at the links I provided in c9, I don't see much of a regression risk at all. The only code that actually implements onCalendarPrefSet is calendar-management.js, which sets up the color and name of the calendar. If the information doesn't change, there is no need to update that information.

My motivation for this bug is bug 392028, which without a patch like the above ends up continually committing the calendar info. I would have to check if each pref value has changed before setting, which will bloat the code a bit.

Comment 12

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 281143 [details] [diff] [review]
onCalendarPrefChanged approach - v1

mvl, no hard feelings on me moving the review to daniel please. We want to spin rc1 on monday, and I'd like to have this changed for 0.7. Since there is not much code that depends on this change, I think its viable to take this patch and if needed discuss any changes after 0.7 is out of the door.

If you have any suggestions tomorrow, feel free to comment.

Comment 13

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: onCalendarPrefChanged approach - v2

Just to make sure, here an updated patch with the missing files patched.

Comment 14

[Daniel Boelzle [:dbo]]

Comment on attachment 281961 [details] [diff] [review]
onCalendarPrefChanged approach - v2

r=dbo

Comment 15

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH.