Tuesday, 2021-06-08

*** jrt is now known as Guest7402002:49
flypig:D Thanks for the prod attah. I did get working aarch64 builds of all my apps working over the weekend, but didn't get a chance to upload them yet. I'll must do that...06:29
dcalisteHello chriadam, how are you ?07:02
chriadamhi dcaliste, well thanks.  how are you?07:02
dcalisteAllright let say...07:04
dcalisteI've looked this week at cleaning and modernizing what can be cleaned and modernized in the calendar stack.07:05
dcalisteYou may have noticed https://git.sailfishos.org/mer-core/buteo-sync-plugin-caldav/merge_requests/8107:06
dcalisteIt's removing some lines that I added a long time ago when we were comparing the incidence on disk with the received one to detect changes.07:06
dcalisteThis comparison is not done anymore and these lines are now obsolete in my opinion.07:06
chriadamlet me check07:09
chriadamadded one comment there07:13
chriadamI guess the final goal is to avoid doing any sort of modifications when exporting?07:14
dcalisteYes, that's the idea.07:14
dcalisteI will answer your comment in Gitlab for reference, but quickly here :07:15
dcalisteyes, we were doing comparison because adding etag made event reported as modified at the next sync.07:15
dcalisteSo we were downloading every event reported as modified and do a comparison.07:15
dcalisteThere were various tricks to make the comparison works. The alarm offset definition was one, because an alarm on date was parsed by iCal as a 0 offset by day, while we were storing this as a 0 offset by seconds.07:17
dcalisteOf course the two offsets are the same, but they were reported as different by kcalcore.07:17
dcalisteThus the patch there at that moment to transform every 0 offset by seconds into by day.07:18
dcalisteThis is not necessary anymore.07:18
chriadamhmm.  does this mean that our backend isn't fulfilling the contract which kcalcore expects?  or am I misunderstanding?07:18
dcalisteWhat do you mean ?07:18
chriadamwell, I may be misunderstanding, but it seems to me: the modification is still necessary, if we ever want to compare two events07:18
chriadamit's just that now, we have less need to compare two events07:18
dcalisteWe never need to compare two events ;)07:19
chriadamwhat if, for example, the user tries to import an event from .ics file twice?  I guess it will fail the second time due to UID uniqueness07:20
chriadamis there really no case where we ever want to compare two events?  interesting.07:20
dcalisteIndeed, that's it.07:21
dcalisteAs far as I can say, there is no need anymore to compare events in Caldav sync plugin.07:21
dcalisteI made sure that we don't actually want to compare events because it created so many bugs and corner cases.07:22
chriadamflypig: can you think of any need to compare events (e.g. local and remote, to perform change detection, rather than relying on the modifiedIncidences() etc methods)?07:22
chriadame.g. in exchange plugin or google plugin?  I hope the answer is "no"07:22
dcalisteWell, this code I propose to remove is for caldav sync plugin only.07:23
flypigI'll just need to catch up and think about that.07:23
dcalisteBut it doesn't hurt to think if we can have similar cases in other plugins.07:23
chriadamoh indeed, this is in caldav repo!  oops07:24
chriadamfor some reason I thought this was something deeper07:24
flypigThere are lots of places where we compare events during sync, but it's usually comparing something received from the server with something in the calendar, which I guess is a separate issue.07:26
dcalisteflypig : well, we were doing actually this in caldav and it required to apply such tricks like the one I propose to remove now, to avoid having spurious modifications.07:27
dcalisteIt was not really an issue but it causes a lot of useless transfer since an event in that category would always be detected as modified and resent at each sync.07:28
flypigYes, I can see the problem.07:28
flypigI guess it doesn't relate to the caldav PR you're looking at, but the example I'm thinking of was for Google sync. I may be misremembering... just checking.07:29
chriadamI mean, if we can move Google sync plugin in the right direction, all the better.  but when I originally asked the question, I had misunderstood and thought that Damien's PR would affect more than just caldav repo07:30
flypigYeah, understood. So not really an essential question for right now any more.07:31
dcalisteNow that mKCal modifiedIncidences() can be relied on (because we can save whatever datetime we want as lastModified field and because we can even now stop the extendedcalendar to mark event as modified for every changes), normally it's safe not to do comparison anymore.07:31
dcalisteAnd just rely on this method for local mods and on etag comparisons for remote ones.07:31
flypigIt might be good to see whether google calendar sync should be updated given this.07:33
dcalistechriadam, I agree we your last comment in the caldav MR.07:33
chriadamI will create a bz bug to track this work07:34
chriadamand we can use that bug in dcaliste's caldav PR also for CI thing07:34
flypigJust for info, in the google calendar plugin there are special methods for comparing KCalendarCore::Incidences07:36
flypigIs that the sort of stuff that could potentially be removed?07:37
dcalisteflypig, ah right. In the Caldav one, we were using KCalCore::Incidence::operator==(), so every minor bit of every components like alarms and so one were required to be exactly the same. Thus a lot of tricks to ensure that outside KCalCore.07:38
dcalisteflypig, I need to see why the comparison is done at the first place.07:38
dcalisteMaybe the Google workflow requires it, but I'm doubtful actually since a CalDAV workflow, with proper etags doesnot require any comparisons.07:39
flypigIt ends up used in a method called "localModificationIsReal", used to avoid "spurious modifications"07:39
dcalisteJust looking at the name, I guess it is the same problem we had in Caldav before.07:39
dcalisteOn a sync, importing the remote modifications marks the events as modified in the next sync.07:40
chriadamcreated JB#54581 to track this work07:40
dcalisteNormally, I've recently added upstream in KCalendarCore a way to actually update an incidence without triggerring the modification handler that will later mark it as modified in mKCal.07:41
dcalisteSo one can now locally update incidence from upstream data being sure that the incidence won't be reported as modified in the next sync. Removing the need to actually do any comparison.07:42
dcalisteThank you chriadam, I'm adding the JB bug to the commit inn Caldav plugin.07:42
dcalisteflypig, see https://git.sailfishos.org/mer-core/buteo-sync-plugin-caldav/commit/1c923899dbce4442a9067ef1f1688ab2f085a05307:43
flypigThere are comments in the code about it being needed for a "timestamp resolution issue", which sounds like it could be similar.07:43
flypigYeah, in all cases it there are comments suggesting it's related to timestamp resolution issues. So maybe it can be fixed in a similar way to how you've done it for caldav.07:45
flypigThat's be nice.07:45
chriadamregarding the QMF patch, a couple of issues: 1) the version is 6.0.0 now but we should change that to 5.6.3 as we are still using Qt 5.6 on device etc, and the content isn't the same as upstream 6.x (just closer than it was previous)07:46
chriadam2) there are some unpackaged .cmake files which it complains about - I didn't know we'd added any cmake things yet?07:46
chriadamflypig: I cc'd you to that JB#54581 bug, I guess it's something you could raise in the iteration planning07:47
dcalisteabout 1) you're right. I'm going to revert this one also.07:47
flypigThanks chriadam, and good idea.07:48
dcalisteabout 2) my bad, I think I just tried to "make" the project and not actually create the rpms. I'm going to check.07:48
dcalisteStrange about cmake thingies, I didn't remember we put any.07:48
chriadamyeah, me either, makes me wonder if I did something wrong07:48
chriadammy process was jsut:07:48
chriadamgit reset --hard dcaliste/qt6 ; mb2 -t jolla-armv7hl build -d -j5 -p07:49
dcalisteYeh, I should have done the same, but it seems that I stopped at the debugging part, just tying "make" inside sb2 target at the root directory...07:50
dcalisteI'll do it with mb2 also and correct what needs to be corrected.07:50
chriadamwell, I'm just wondering where the cmake files actually come from haha07:51
chriadamaside from that, I meant to run the actual unit tests on device, but building took longer than I expected and I ran out of time before the meeting07:52
chriadamI apologise for that, my poor planning07:52
chriadamI will test them this week07:52
chriadamregarding branching: my understanding is that is still happening on Thursday07:53
dcalisteNo problem, it's really not in a hurry.07:53
chriadamso PRs can be merged after then07:53
dcalisteSure, no problem neither with this planning. No MRs are dealing with bugs anyway.07:53
chriadamregarding the github transition: I have no idea what the current timeline for that is.  with luck we'll be able to merge a bunch of PRs after branching but before the transition switch is flipped07:53
chriadambut let's see07:53
dcalisteIf you still have a bit of time, I would like to discuss this MR : https://git.sailfishos.org/mer-core/mkcal/merge_requests/6607:54
chriadamof course07:54
dcaliste(no problem with the github transition, if necessary I'll open MRs there)07:54
dcalisteThis mKCal thing is also a cleaning patch but much more intrusive in term of API and consequences.07:55
dcalisteIt's the patch that remove the necessity to make any exception as ExDate in the parent (which was an RFC violation).07:56
dcalisteWe discussed this a long time ago together, that initially it was a smart idea to add exception as exdate in the parent, because like that timesInInterval() return a proper list of date times.07:56
dcalisteWithout needing to filter out the exception occurences.07:57
chriadamgrepping for rawExpanded doesn't show anything in sailfish-eas, at least07:57
dcalisteThis method is used for displaying events normally, so I don't expect it to be used in sync plugins. But thank you for checking, it's safer.07:58
dcalisteBut adding exeptions as exdate requires to add in the sync plugins a lot of boiler plate to add or remove them when speaking with the outter world that don't use this convention.07:59
chriadamyeah, that exdate thing always annoyed me07:59
chriadamI guess I haven't fully had time to digest this change, but my initial thoughts are:07:59
chriadam1) great, if it only affects rawExpandedEvents() and timesInInterval(), then that at least is a fairly small "surface"08:00
chriadam2) but we will need to check if partners are using these APIs in any of their things, before we change those ones.  I will ask pvuorela to check.08:00
chriadam3) overall, I agree that getting rid of the special exdate handling from our sync plugins would be greatly beneficial.08:01
chriadamthings I'm unsure about:08:01
chriadama) you mention the expansion only makes sense in the system timezone -> I don't really understand this08:02
chriadamb) is timesInInterval() basically just an internal detail?08:02
chriadamanyway, I will add some comments to the PR, and ask Pekka to take a look also08:02
dcalistefor b), yes, that's what I understand from the KCalendarCore API. This method should not be used outside KCalendarCore, except if you know what you're doing. The OcuurrenceIterator is the convenient API for everyone and for every time you want to display events taking into account periodicity and exceptions.08:04
dcalisteAbout a), it's more a technical requirement. If you allow to get occurrences in another time zone than the system one, then you need to do time comparison by hand (as you did in the original routine) when dealing with LocalTime.08:05
dcalisteBut it makes no sense to display occurrences in another time zone than the one of the device.08:06
dcalisteI don't want to ask my calendar to display my schedule as if I were in Asia while I'm in Paris. Printed times for events would be very confusing.08:07
dcalisteOr if I want to know when will be a meeting for me at 2pm in Paris for colleagues in Ho Chi Minh, then yes it's a valid usecase, but rawExpandedEvents() is not the method to be called for this.08:08
chriadamyeah.  I mean, I can see a use case for it (e.g. "I'm going on a business trip to finland next week; show me my calendar schedule, in finnish timezone, so I can see what time I have meetings etc in that tz, next week" but it's a bit of a stretch08:08
dcalisteYes, exactly.08:08
dcalisteYou can still fake it (like in the tests) by actually changing the TZ environment variable.08:09
dcalisteSo, it's still possible, just a bit less direct than an argument. But the coding is *much* simpler without the argument.08:09
dcalisteOf course you can change locally the TZ in a calling routine without affecting the full device.08:10
dcalisteI've changed the tests to actually run like that to test that local times are properly expanded in various time zones.08:11
chriadamoverall: I agree08:11
dcalisteWhat is nice is that we don't need to migrate the database.08:12
chriadamI'd appreciate flypig's input on that one too, of course, and pvuorela will have to ask OMP etc whether changes might cause issues.08:12
flypig👍 I'll take a look.08:13
chriadamthanks :-)08:13
dcalisteAlready saved parent events with the spurious exdate won't confuse the OccurrenceIterator, just that the filtering out will be done in the internal call to timesInInterval() instead of in the iterator itself.08:13
dcalisteThank you for the internal work that you will have to do.08:13
*** frinring_ is now known as frinring08:14
chriadamnow I'm confused a bit08:14
dcalisteI tried not to touch the API, but I prefer to make it simpler. If we need to keep the API, I can wrap the TZ trick inside the call.08:14
chriadamwe will have parents with spurious exdates saved in them.  does this mean we still need the exdate handling in the sync plugins?08:15
dcalisteYes sadly yes... For exportation only. We can remove the code that adds the exdate on reception though.08:15
chriadamwell, better than nothing.  we can add a big code comment explaining what the export-side code is for, and to remind us to remove in a couple of years time or something08:16
dcalisteOtherwise if we want to cure the exportation, we'll have to migrate the database content. I think I will do it for Caldav sync because I've already a branch that's going to do it for all other leftovers.08:17
chriadammigrating the db content makes the most sense (if we can do the changes to all sync plugins "atomically" in one release)08:17
dcalisteYes, maybe at one moment when we will revamp the schemas.08:18
dcalisteAs I said, keeping the spurious exdates will (should) not do any harm at the moment, besides keeping the exportation code that basically works.08:19
chriadamI will add a comment to the PR for discussion, but let's just consider it as discussion for now ;-)08:20
flypigI'm a little behind on this, so sorry if you already covered it, but will this mean some there will be some exDateTime recurrenceIds (for old events), but not others (for new events)?08:21
dcalisteSure, of course, this one may have more consequences than the Buteo caldav one, so let do it cautiously !08:21
dcalisteflypig, exact.08:22
dcalisteThat's why we need to keep the exportation filtering code.08:22
flypigOkay. That may have implications for the Google sync, which performs a bit of a dance to add/remove exdates from events. That could be brittle.08:22
chriadamand it's why I'd prefer to do a db migration, but it requires coordinating all of the changes together (i.e. caldav+google+eas+...)08:22
dcalisteflypig, if the MR is applied, you can remove all code from plugin on import (no need to add the exceptions as exdate, no need to treat the exceptions beofre the parent...), but you'll have to keep the code removing spurious exdate on export.08:23
flypigYes, makes sense. Thanks.08:24
dcalisteThe export code doesn't have to be change, for "new" event, it will just do nothing.08:24
dcalisteSo actually, if everything goes well, it should just be code removal.08:24
flypigIt would be nice if they both matched; it should indeed simplify things.08:25
dcaliste(sorry you should have read no need to treat the parent before the exceptions and not the reverse)08:25
flypigFor downloads at least; probably the ordering is still important for uploads.08:26
dcalisteIndeed, for download only.08:27
chriadamthanks for looking into that one - definitely will simplify a lot of code if we can get it all done08:30
chriadamflypig: again, maybe you could raise that one for us at iteration planning?08:30
chriadamwould be good if we could get a green light to do it all at once08:30
flypigchriadam: yes, good idea.08:30
chriadamthank you08:30
chriadamwas there anything else to discuss today?  I guess we haven't made any progress on your other PRs currently as we're waiting for branching and busy with the last minute feature things needed in before branching at the moment08:31
flypigPerhaps we can raise it on Monday too so it's on the radar. I may miss something without your input.08:31
dcalisteThank both of you.08:32
chriadamthank you very much for your efforts, as always, dcaliste.  very much appreciated!08:32
flypigAnd the same.08:33
chriadamif nothing else, let's end the meeting for tonight - have a great week!08:33
dcalisteI don't have anything else to discuss, besides thanking you again for the support.08:33
chriadamgood night!08:33
dcalisteI which you a good evening chriadam08:33
dcalisteHave a nice day flypig.08:33
* chriadam -> away08:33
flypigAnd you both too!08:33
Aardaccount list13:09
malsashikknox: hi18:35
sashikknox<mal "sashikknox: hi"> hi )19:24
sashikknox> <@freenode_mal:matrix.org> sashikknox: hi19:24
sashikknox * hi )19:24
sashikknoxcan you test aarch64 quake2 ? i rebuild it, but cant tet )19:24
malsashikknox: ok, I can test but I was pinging you about the submodule in sdl PR19:24
malsashikknox: maybe we can add the submodule already now, so remove the old SDL2 folder and add this as submodule https://github.com/sailfishos-mirror/SDL.git19:25
sashikknox<mal "sashikknox: ok, I can test but I"> ok19:25
sashikknox<mal "sashikknox: maybe we can add the"> sounds nice, as for me, i just want see in SailfishOS SDL2 with fully wayland support, than i can public Quake2 on harbour )19:26
sashikknox<mal "sashikknox: maybe we can add the"> is SDL2 already release 2.15 ?19:27
malno, they haven't released new version yet19:27
malsashikknox: remember to checkout the correct revision of the submodule when adding it19:28
sashikknox<mal "sashikknox: remember to checkout"> ok, can i add latest commit from master as current submodule state?19:30
malhmm, usually we prefer release versions19:30
malwith patches on top19:30
sashikknox<mal "with patches on top"> ok? no problem? live patches as is, and add sobmodule to 2.14 state19:31
sashikknox> <@freenode_mal:matrix.org> with patches on top19:31
sashikknox * ok, no problem, live patches as is, and add sobmodule to 2.14 state19:31
Nico-old-defunctsashikknox: Be carful with the edits and replies in this room, so that you won't collect the wrath of the IRC users <319:32
malyes, to otherwise the same PR as now but a new commit which removes the old SDL2 folder and adds the submodule, I have previous done removal of the old sources and adding submodule in separate commits for more clear history19:33
sashikknox<Nico-old-defunct "sashikknox: Be carful with the e"> oh, sorry, i forgot it in mattrix bridge )19:33
Nico-old-defunctWell, not that I care, I'm on the right side of the bridge :319:33
sashikknox<mal "yes, to otherwise the same PR as"> ok, i'll do it19:33
malsashikknox: was there some magic for quake2 to get it to scale properly?19:57
malsashikknox: I see this "Creating a window with width=320 height=240 fullscreen=0"19:59
sashikknox<mal "sashikknox: was there some magic"> you want 1:1 scale? ... sorry ) but it hardcoded ) in next release i'll add it as option in videosettings19:59
malsashikknox: I mean the game is just a small area at top of the screen20:00
sashikknox<mal "sashikknox: I see this "Creating"> hmm.. itts little bit strange, too smal, let me check in on arm20:00
malI'll try on some arm device also20:00
sashikknox<mal "sashikknox: I mean the game is j"> looks like it bad  build , i'll check oit now... maybe miss some define for SailfishOS ///20:01
malsashikknox: also it the game was not shown in landscape20:02
sashikknox<mal "sashikknox: also it the game was"> yeah, looks like miss some build flags...20:02
sashikknox<mal "yes, to otherwise the same PR as"> i finish with PR/MR20:04
sashikknox<mal "sashikknox: also it the game was"> yes, i just disable all SailfishOS fetures in this build... an forgot about it, i'll rebuild it now, and update RPM on OpenRepos20:06
malsashikknox: a small mistake in submodule, please use https://github.com/sailfishos-mirror/SDL.git as the url, not the git url20:06
sashikknox<mal "sashikknox: a small mistake in s"> ok20:07
malnot sure if these are needed but I usually run these if changing submodule url20:08
malgit submodule sync20:08
malgit submodule update --init --recursive --remote20:08
malafter that still make sure the revision is correct20:08
sashikknox<mal "after that still make sure the r"> check now, all looks correct20:12
malseems better now, at least it started building20:16
sashikknox<mal "sashikknox: also it the game was"> please, test again aarch64 build of Quake2 ( on openrepos )20:16
malsashikknox: I don't see aarch64 build in there anymore20:17
malnow it appeared20:18
sashikknox<mal "sashikknox: I don't see aarch64 "> check now, maby some timout on openrepos20:19
sashikknoxQuake2, oh no ... something went wrong... bugos build... just check if it work, but you cant paly it normal...20:23
malsashikknox: seems to work, I can also play it20:27
sashikknox<mal "sashikknox: seems to work, I can"> yeah, some my mistake, i'll fix it soon, now i search and error )20:27
sashikknox> <@freenode_mal:matrix.org> sashikknox: seems to work, I can also play it20:28
sashikknox * yeah, some my mistake, i'll fix it soon, now i search error )20:28
malplease no replys like that as mentioned earlier :)20:28
maljust write the comment normally, we'll get the context20:29

Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!