search

elementary / calendar

Desktop calendar app designed for elementary OS
https://elementary.io
GNU General Public License v3.0
130 stars 39 forks source link

Revert "utils: Workaround wrong ICal.Time.get_timezone() binding to fix memleak (#719)" #722

Closed tintou closed 2 years ago

tintou commented 2 years ago

Also add code to release the internal cache at the end of the program

jeremypw commented 2 years ago

@mcclurgm Could you just check this as you know more about timezone and ICal than I do?

mcclurgm commented 2 years ago

This is really exposing some more problems we have in the icaltime_get_timezone function. The unreachable code you deleted would be necessary to account for failed timezone creations (like anything that doesn't have the Olson TZID format), but glib only ever fails silently so I never accounted for that. Some tech debt that was my fault. I think it would be a good thing to add it back at some point, unless we can properly get rid of GLib timezones altogether, since if there's one thing I've learned trying to deal with converting back and forth it's that the whole process is messy and full of holes, and best avoided.

Otherwise, it seems like the original #719 was accounting for the memory issue the wrong way, and the proper way is to free libical's global timezone cache when the app exits. Is that correct? I'm not familiar with Vala manual memory management, but it looks like the old PR was improperly adding a free call, which I suppose makes sense to remove.

Otherwise, the changes made to icaltime_get_timezone make sense given what I said above. I'll trust you about free_global_objects I suppose. So based on what I can understand, this makes sense to me.

jeremypw commented 2 years ago

@mcclurgm So you would not anticipate any serious regressions from this? I am not sure how to test the timezone stuff.

mcclurgm commented 2 years ago

Compared to the state before #719, I don't expect any regressions. The changes to icaltime_get_timezone should have no effect (any removed code was either duplicate or unreachable), and otherwise the only change is adding the call to free_global_objects. I expect no timezone regressions. We also have tests that should indicate regressions if I wrote them right.

Compared to the state after #719, that's far enough outside my knowledge area I don't know. Hopefully that's enough info for you.

jeremypw commented 2 years ago

Thanks @mcclurgm, I think I can approve on that basis.