| *** jrt is now known as Guest74020 | 02: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 |
|---|---|---|
| dcaliste | Hello chriadam, how are you ? | 07:02 |
| chriadam | hi dcaliste, well thanks. how are you? | 07:02 |
| dcaliste | Allright let say... | 07:04 |
| dcaliste | I've looked this week at cleaning and modernizing what can be cleaned and modernized in the calendar stack. | 07:05 |
| dcaliste | You may have noticed https://git.sailfishos.org/mer-core/buteo-sync-plugin-caldav/merge_requests/81 | 07:06 |
| dcaliste | It'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 |
| dcaliste | This comparison is not done anymore and these lines are now obsolete in my opinion. | 07:06 |
| chriadam | let me check | 07:09 |
| chriadam | added one comment there | 07:13 |
| chriadam | I guess the final goal is to avoid doing any sort of modifications when exporting? | 07:14 |
| dcaliste | Yes, that's the idea. | 07:14 |
| dcaliste | I will answer your comment in Gitlab for reference, but quickly here : | 07:15 |
| dcaliste | yes, we were doing comparison because adding etag made event reported as modified at the next sync. | 07:15 |
| dcaliste | So we were downloading every event reported as modified and do a comparison. | 07:15 |
| dcaliste | There 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 |
| dcaliste | Of course the two offsets are the same, but they were reported as different by kcalcore. | 07:17 |
| dcaliste | Thus the patch there at that moment to transform every 0 offset by seconds into by day. | 07:18 |
| dcaliste | This is not necessary anymore. | 07:18 |
| chriadam | hmm. does this mean that our backend isn't fulfilling the contract which kcalcore expects? or am I misunderstanding? | 07:18 |
| dcaliste | What do you mean ? | 07:18 |
| chriadam | well, I may be misunderstanding, but it seems to me: the modification is still necessary, if we ever want to compare two events | 07:18 |
| chriadam | it's just that now, we have less need to compare two events | 07:18 |
| dcaliste | We never need to compare two events ;) | 07:19 |
| chriadam | what 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 uniqueness | 07:20 |
| chriadam | is there really no case where we ever want to compare two events? interesting. | 07:20 |
| dcaliste | Indeed, that's it. | 07:21 |
| dcaliste | As far as I can say, there is no need anymore to compare events in Caldav sync plugin. | 07:21 |
| dcaliste | I made sure that we don't actually want to compare events because it created so many bugs and corner cases. | 07:22 |
| chriadam | flypig: 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 |
| chriadam | e.g. in exchange plugin or google plugin? I hope the answer is "no" | 07:22 |
| dcaliste | Well, this code I propose to remove is for caldav sync plugin only. | 07:23 |
| flypig | I'll just need to catch up and think about that. | 07:23 |
| dcaliste | But it doesn't hurt to think if we can have similar cases in other plugins. | 07:23 |
| chriadam | oh indeed, this is in caldav repo! oops | 07:24 |
| chriadam | for some reason I thought this was something deeper | 07:24 |
| flypig | There 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 |
| dcaliste | flypig : 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 |
| dcaliste | It 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 |
| flypig | Yes, I can see the problem. | 07:28 |
| flypig | I 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 |
| chriadam | I 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 repo | 07:30 |
| flypig | Yeah, understood. So not really an essential question for right now any more. | 07:31 |
| dcaliste | Now 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 |
| dcaliste | And just rely on this method for local mods and on etag comparisons for remote ones. | 07:31 |
| flypig | It might be good to see whether google calendar sync should be updated given this. | 07:33 |
| chriadam | yes. | 07:33 |
| dcaliste | chriadam, I agree we your last comment in the caldav MR. | 07:33 |
| chriadam | I will create a bz bug to track this work | 07:34 |
| chriadam | and we can use that bug in dcaliste's caldav PR also for CI thing | 07:34 |
| chriadam | sec | 07:34 |
| flypig | Just for info, in the google calendar plugin there are special methods for comparing KCalendarCore::Incidences | 07:36 |
| flypig | https://git.sailfishos.org/mer-core/buteo-sync-plugins-social/blob/master/src/google/google-calendars/googlecalendarincidencecomparator.h | 07:36 |
| flypig | Is that the sort of stuff that could potentially be removed? | 07:37 |
| dcaliste | flypig, 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 |
| dcaliste | flypig, I need to see why the comparison is done at the first place. | 07:38 |
| dcaliste | Maybe the Google workflow requires it, but I'm doubtful actually since a CalDAV workflow, with proper etags doesnot require any comparisons. | 07:39 |
| flypig | It ends up used in a method called "localModificationIsReal", used to avoid "spurious modifications" | 07:39 |
| flypig | https://git.sailfishos.org/mer-core/buteo-sync-plugins-social/blob/master/src/google/google-calendars/googlecalendarsyncadaptor.cpp#L806 | 07:39 |
| dcaliste | Just looking at the name, I guess it is the same problem we had in Caldav before. | 07:39 |
| dcaliste | On a sync, importing the remote modifications marks the events as modified in the next sync. | 07:40 |
| chriadam | created JB#54581 to track this work | 07:40 |
| dcaliste | Normally, 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 |
| dcaliste | So 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 |
| dcaliste | Thank you chriadam, I'm adding the JB bug to the commit inn Caldav plugin. | 07:42 |
| chriadam | ty | 07:42 |
| dcaliste | flypig, see https://git.sailfishos.org/mer-core/buteo-sync-plugin-caldav/commit/1c923899dbce4442a9067ef1f1688ab2f085a053 | 07:43 |
| flypig | There are comments in the code about it being needed for a "timestamp resolution issue", which sounds like it could be similar. | 07:43 |
| flypig | Yeah, 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 |
| flypig | That's be nice. | 07:45 |
| chriadam | regarding 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 |
| chriadam | 2) there are some unpackaged .cmake files which it complains about - I didn't know we'd added any cmake things yet? | 07:46 |
| chriadam | flypig: I cc'd you to that JB#54581 bug, I guess it's something you could raise in the iteration planning | 07:47 |
| dcaliste | about 1) you're right. I'm going to revert this one also. | 07:47 |
| flypig | Thanks chriadam, and good idea. | 07:48 |
| dcaliste | about 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 |
| dcaliste | Strange about cmake thingies, I didn't remember we put any. | 07:48 |
| chriadam | yeah, me either, makes me wonder if I did something wrong | 07:48 |
| chriadam | my process was jsut: | 07:48 |
| chriadam | git reset --hard dcaliste/qt6 ; mb2 -t jolla-armv7hl build -d -j5 -p | 07:49 |
| dcaliste | Yeh, 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 |
| dcaliste | I'll do it with mb2 also and correct what needs to be corrected. | 07:50 |
| chriadam | well, I'm just wondering where the cmake files actually come from haha | 07:51 |
| chriadam | aside 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 meeting | 07:52 |
| chriadam | I apologise for that, my poor planning | 07:52 |
| chriadam | I will test them this week | 07:52 |
| chriadam | regarding branching: my understanding is that is still happening on Thursday | 07:53 |
| dcaliste | No problem, it's really not in a hurry. | 07:53 |
| chriadam | so PRs can be merged after then | 07:53 |
| dcaliste | Sure, no problem neither with this planning. No MRs are dealing with bugs anyway. | 07:53 |
| chriadam | regarding 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 flipped | 07:53 |
| chriadam | but let's see | 07:53 |
| dcaliste | If you still have a bit of time, I would like to discuss this MR : https://git.sailfishos.org/mer-core/mkcal/merge_requests/66 | 07:54 |
| chriadam | of course | 07:54 |
| dcaliste | (no problem with the github transition, if necessary I'll open MRs there) | 07:54 |
| dcaliste | This mKCal thing is also a cleaning patch but much more intrusive in term of API and consequences. | 07:55 |
| dcaliste | It's the patch that remove the necessity to make any exception as ExDate in the parent (which was an RFC violation). | 07:56 |
| dcaliste | We 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 |
| dcaliste | Without needing to filter out the exception occurences. | 07:57 |
| chriadam | grepping for rawExpanded doesn't show anything in sailfish-eas, at least | 07:57 |
| dcaliste | This 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 |
| dcaliste | But 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 |
| chriadam | yeah, that exdate thing always annoyed me | 07:59 |
| chriadam | I guess I haven't fully had time to digest this change, but my initial thoughts are: | 07:59 |
| chriadam | 1) great, if it only affects rawExpandedEvents() and timesInInterval(), then that at least is a fairly small "surface" | 08:00 |
| chriadam | 2) 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 |
| chriadam | 3) overall, I agree that getting rid of the special exdate handling from our sync plugins would be greatly beneficial. | 08:01 |
| chriadam | things I'm unsure about: | 08:01 |
| chriadam | a) you mention the expansion only makes sense in the system timezone -> I don't really understand this | 08:02 |
| chriadam | b) is timesInInterval() basically just an internal detail? | 08:02 |
| chriadam | anyway, I will add some comments to the PR, and ask Pekka to take a look also | 08:02 |
| dcaliste | for 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 |
| dcaliste | About 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 |
| dcaliste | But it makes no sense to display occurrences in another time zone than the one of the device. | 08:06 |
| dcaliste | I 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 |
| dcaliste | Or 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 |
| chriadam | yeah. 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 stretch | 08:08 |
| dcaliste | Yes, exactly. | 08:08 |
| dcaliste | You can still fake it (like in the tests) by actually changing the TZ environment variable. | 08:09 |
| chriadam | mm. | 08:09 |
| dcaliste | So, it's still possible, just a bit less direct than an argument. But the coding is *much* simpler without the argument. | 08:09 |
| dcaliste | Of course you can change locally the TZ in a calling routine without affecting the full device. | 08:10 |
| dcaliste | I've changed the tests to actually run like that to test that local times are properly expanded in various time zones. | 08:11 |
| chriadam | overall: I agree | 08:11 |
| dcaliste | What is nice is that we don't need to migrate the database. | 08:12 |
| chriadam | I'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 |
| chriadam | thanks :-) | 08:13 |
| dcaliste | Already 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 |
| dcaliste | Thank you for the internal work that you will have to do. | 08:13 |
| *** frinring_ is now known as frinring | 08:14 | |
| chriadam | now I'm confused a bit | 08:14 |
| dcaliste | I 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 |
| chriadam | we will have parents with spurious exdates saved in them. does this mean we still need the exdate handling in the sync plugins? | 08:15 |
| dcaliste | Yes sadly yes... For exportation only. We can remove the code that adds the exdate on reception though. | 08:15 |
| chriadam | indeed | 08:16 |
| chriadam | well, 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 something | 08:16 |
| dcaliste | Otherwise 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 |
| chriadam | migrating the db content makes the most sense (if we can do the changes to all sync plugins "atomically" in one release) | 08:17 |
| dcaliste | Yes, maybe at one moment when we will revamp the schemas. | 08:18 |
| dcaliste | As 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 |
| chriadam | indeed. | 08:20 |
| chriadam | I will add a comment to the PR for discussion, but let's just consider it as discussion for now ;-) | 08:20 |
| flypig | I'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 |
| dcaliste | Sure, of course, this one may have more consequences than the Buteo caldav one, so let do it cautiously ! | 08:21 |
| dcaliste | flypig, exact. | 08:22 |
| dcaliste | That's why we need to keep the exportation filtering code. | 08:22 |
| flypig | Okay. 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 |
| chriadam | and 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 |
| dcaliste | flypig, 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 |
| flypig | Yes, makes sense. Thanks. | 08:24 |
| dcaliste | The export code doesn't have to be change, for "new" event, it will just do nothing. | 08:24 |
| dcaliste | So actually, if everything goes well, it should just be code removal. | 08:24 |
| flypig | It 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 |
| flypig | For downloads at least; probably the ordering is still important for uploads. | 08:26 |
| dcaliste | Indeed, for download only. | 08:27 |
| chriadam | thanks for looking into that one - definitely will simplify a lot of code if we can get it all done | 08:30 |
| chriadam | flypig: again, maybe you could raise that one for us at iteration planning? | 08:30 |
| chriadam | would be good if we could get a green light to do it all at once | 08:30 |
| flypig | chriadam: yes, good idea. | 08:30 |
| chriadam | thank you | 08:30 |
| chriadam | was 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 moment | 08:31 |
| flypig | Perhaps we can raise it on Monday too so it's on the radar. I may miss something without your input. | 08:31 |
| chriadam | sure | 08:31 |
| dcaliste | Thank both of you. | 08:32 |
| chriadam | thank you very much for your efforts, as always, dcaliste. very much appreciated! | 08:32 |
| flypig | And the same. | 08:33 |
| chriadam | if nothing else, let's end the meeting for tonight - have a great week! | 08:33 |
| dcaliste | I don't have anything else to discuss, besides thanking you again for the support. | 08:33 |
| chriadam | :-) | 08:33 |
| chriadam | good night! | 08:33 |
| dcaliste | I which you a good evening chriadam | 08:33 |
| dcaliste | Have a nice day flypig. | 08:33 |
| * chriadam -> away | 08:33 | |
| flypig | And you both too! | 08:33 |
| Aard | account list | 13:09 |
| mal | sashikknox: hi | 18:35 |
| sashikknox | <mal "sashikknox: hi"> hi ) | 19:24 |
| sashikknox | > <@freenode_mal:matrix.org> sashikknox: hi | 19:24 |
| sashikknox | * hi ) | 19:24 |
| sashikknox | can you test aarch64 quake2 ? i rebuild it, but cant tet ) | 19:24 |
| mal | sashikknox: ok, I can test but I was pinging you about the submodule in sdl PR | 19:24 |
| mal | sashikknox: 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.git | 19:25 |
| sashikknox | <mal "sashikknox: ok, I can test but I"> ok | 19: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 |
| mal | no, they haven't released new version yet | 19:27 |
| mal | sashikknox: remember to checkout the correct revision of the submodule when adding it | 19:28 |
| sashikknox | <mal "sashikknox: remember to checkout"> ok, can i add latest commit from master as current submodule state? | 19:30 |
| mal | hmm, usually we prefer release versions | 19:30 |
| mal | with patches on top | 19:30 |
| sashikknox | <mal "with patches on top"> ok? no problem? live patches as is, and add sobmodule to 2.14 state | 19:31 |
| sashikknox | > <@freenode_mal:matrix.org> with patches on top | 19:31 |
| sashikknox | * ok, no problem, live patches as is, and add sobmodule to 2.14 state | 19:31 |
| Nico-old-defunct | sashikknox: Be carful with the edits and replies in this room, so that you won't collect the wrath of the IRC users <3 | 19:32 |
| Nico-old-defunct | *attract | 19:33 |
| mal | yes, 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 history | 19:33 |
| sashikknox | <Nico-old-defunct "sashikknox: Be carful with the e"> oh, sorry, i forgot it in mattrix bridge ) | 19:33 |
| Nico-old-defunct | Well, not that I care, I'm on the right side of the bridge :3 | 19:33 |
| sashikknox | <mal "yes, to otherwise the same PR as"> ok, i'll do it | 19:33 |
| mal | sashikknox: was there some magic for quake2 to get it to scale properly? | 19:57 |
| mal | sashikknox: 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 videosettings | 19:59 |
| mal | sashikknox: I mean the game is just a small area at top of the screen | 20:00 |
| sashikknox | <mal "sashikknox: I see this "Creating"> hmm.. itts little bit strange, too smal, let me check in on arm | 20:00 |
| mal | I'll try on some arm device also | 20: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 |
| mal | sashikknox: also it the game was not shown in landscape | 20: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/MR | 20:04 |
| mal | ok | 20: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 OpenRepos | 20:06 |
| mal | sashikknox: a small mistake in submodule, please use https://github.com/sailfishos-mirror/SDL.git as the url, not the git url | 20:06 |
| sashikknox | <mal "sashikknox: a small mistake in s"> ok | 20:07 |
| mal | not sure if these are needed but I usually run these if changing submodule url | 20:08 |
| mal | git submodule sync | 20:08 |
| mal | git submodule update --init --recursive --remote | 20:08 |
| mal | after that still make sure the revision is correct | 20:08 |
| sashikknox | <mal "after that still make sure the r"> check now, all looks correct | 20:12 |
| mal | seems better now, at least it started building | 20:16 |
| sashikknox | <mal "sashikknox: also it the game was"> please, test again aarch64 build of Quake2 ( on openrepos ) | 20:16 |
| mal | sashikknox: I don't see aarch64 build in there anymore | 20:17 |
| mal | now it appeared | 20:18 |
| sashikknox | <mal "sashikknox: I don't see aarch64 "> check now, maby some timout on openrepos | 20:19 |
| sashikknox | Quake2, oh no ... something went wrong... bugos build... just check if it work, but you cant paly it normal... | 20:23 |
| mal | sashikknox: seems to work, I can also play it | 20: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 it | 20:28 |
| sashikknox | * yeah, some my mistake, i'll fix it soon, now i search error ) | 20:28 |
| mal | please no replys like that as mentioned earlier :) | 20:28 |
| mal | just write the comment normally, we'll get the context | 20:29 |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!