Tuesday, 2021-05-25

dcalisteHello chriadam. I hope you're well.07:01
chriadamhi dcaliste, yes I am doing well thanks.  how has your week been?07:02
dcalisteIt was good thanks. I've started to adjust Qt6 changes in QMF for backport in Sailfish. Maybe you've seen the MR. I'm sorry I noticed your work on monich's patch only after.07:04
dcalisteI need to update the MR to actually use your modifications instead of my quick compilation fix.07:04
dcalistesee https://git.sailfishos.org/mer-core/messagingframework/merge_requests/5907:05
chriadamI saw the PR - thanks.  I didn't get a chance to review or discuss with Pekka yet.  I don't recall the other patch, let me check07:05
chriadamam I correct in saying: the Qt6 adjust patch shouldn't change any behaviour, but it will make it easier to transition in future once we upgrade Qt?07:06
dcalisteMonich's patch upstream is https://codereview.qt-project.org/c/qt-labs/messagingframework/+/30866507:06
dcalisteThe Qt6 move was required to be able to upstream patches that were pilling up in git.sailfishos.org.07:07
chriadamoh damn I had forgotten about that one.  I was supposed to poke pvuorela and flypig to review that upstream PR so that it could be merged, I think07:07
dcalisteI'm not completely sure that it will help any Qt transition to something strictly lower than 6.0 though.07:07
dcalisteBecause most modifications were just deprecations in Qt5.xx.07:08
chriadamso the main benefit is that if we apply patches locally, they can be simply upstreamed, since they're working off very similar codebase?07:08
dcalisteThat being said, I was suprised to the actual low neumber of revert patches that was needed to compile in SDK.07:09
dcalisteYes, that's the idea to backport to SailfishOS.07:09
chriadam(sorry my memory of these details is all hazy, I have context switched to other things)07:09
dcalisteIt would avoid issues like the problematic rebase of monich's patch.07:10
dcalisteBecause his patch would have been done directly on "master", I mean the upstream one or something that is as close as possible to the upstream one with the Qt5.6 restriction.07:10
chriadamright, makes sense. I've added a comment to the MR for context07:11
chriadamthat one will probably not be merged before we move to github, I guess, so might have to recreate the MR there shortly07:11
chriadampvuorela: ^07:11
dcalisteNo problem to recreate the MR in github when necessary. It's just a matter of push plus some copy-pasting.07:12
chriadamindeed, thanks07:13
chriadamgoing on from last week's topics:07:14
dcalisteI've seen that flypig spent time looking on consequences of the organizer not been attendee patch in mKCal. It's very kind from him.07:15
chriadam1) mkcal MR#63 has some discussion from flypig, my understanding is that he agrees that the patch shouldn't cause issues on EAS side07:15
chriadam2) mkcal MR#65 has some comments from Pekka, I saw.  I think adding proper support for attachments would be nice.07:17
chriadamI guess that one is blocking nqpc#86 currently07:19
dcalisteAbout 1), I guess it's up to you to see when you (as Jolla I mean) want to accept the change. When done, I'll create a MR in caldav sync plugin to remove the unnecessary lines there.07:20
chriadam3) regarding the instanceIdentifier change (e.g. nqpc#84) was there something you were looking into changing with that one, still?  or are we waiting on upstream to discuss / accept something?07:20
chriadamyup, I will poke Pekka about (1) and maybe get that in, during the next day or two I hope.07:21
flypigFor 1, it looks fine for EAS, but I only checked visually and didn't get a chance to test it I'm afraid.07:21
chriadamflypig: true.  maybe we should wait until after 4.2.0 branching07:21
chriadambefore merging07:21
flypigThat sounds safer, although I guess there's still a branching -> releasing gap to test it in.07:22
dcalisteAs you think is better. Waiting after branching causes no problem, since the target of this is only code simplication (hopefully).07:23
chriadamyeah.  my first (risk averse) instinct is to wait, and merge the change next week instead.07:24
dcalisteThe main idea, is to be able to dump any mKCal stored data using the KCalendarCore ICalFormat export function without having to clean-up every incidence first.07:24
dcalisteWhen you look at the IncidenceHandler::incidenceToExport() function, we're not far.07:25
dcalisteThere is still something about the alarm duration that I need to check if we can get rid of it by moving it to mKCal also or somewhere else.07:26
dcalisteAnd then will just remain the UID change, which is due to a restriction in the UNIQUE index of the component database in mKCal :(07:27
chriadamI wonder if we can fix that by making a combination of UID+notebookUid the primary key / uniqueness contraint07:28
chriadamin mkcal07:28
chriadamof course, will then require some migration of existing data step07:28
dcalisteAnd also the EXDate thing for recurring events. Thing that I will try to get rid of in a medium future by modifying the expandedRawEvent() function in mKCal, or even better use the new occurrence iterator from upstream :^D07:29
dcalisteFor the UNIQUE constraint, yes it could be eased by migrating the database to add the notebookid on the constraint.07:29
dcalisteBut I'm not keen to do a migration at the moment. Maybe later when we actually need it to add a missing functionality.07:31
dcalisteAbout your point 2), I've seen the comments from pvuorela (thanks for them) and will address them later today.07:32
dcalisteThere is one point that we may discuss here though, which is the support for attachment for alarms.07:33
dcalisteThe RFC says that ATTACH can be present for VEVENTs, VTODOs and VJOURNALs. Which is covered by the MR.07:33
dcalisteBut it can also be added for VALARMs. Which is not covered at the moment.07:36
chriadamdo we support importing VALARMs currently?07:39
chriadamI wasn't aware we even supported importing VTODO or VJOURNAL entries honestly07:39
chriadamso, this doesn't sound like a problem to me07:40
chriadamor is there something I'm overlooking?07:40
dcalisteThe idea is for this MR to be future compatible.07:40
dcalisteSee my latest comment in this MR.07:40
chriadamah, the alarmId column07:42
dcalisteIndeed. My incline would be not to add a coulumn alarmID that would be de facto exclusive with the componentId one, but use the negative values trick.07:43
dcalisteWith enough comment in the code it could not be that bad.07:44
chriadamoh, is componentId not part of a compound key?07:44
dcalisteNo, the layout of the database is very crude and all cross table is done by hand in the C++ code.07:46
chriadamunsurprising, I suppose07:47
chriadamwell, maybe pvuorela or flypig have some preference.  I don't mind too much, but a negative value does seem slightly ... bad / magical.. I dunno.07:48
dcalisteYou have better knowledge on this than myself. If it looks to surprising then it's not a good solution.07:49
flypigSorry, I'm trailing with this. I'd need to look at the code to have an opinion.07:50
chriadam(referring to mkcal#65)07:50
dcalisteAnd particularly this comment : https://git.sailfishos.org/mer-core/mkcal/merge_requests/65#note_8847007:51
chriadamwell, maybe we can come back to this one next week after some more time for digestion.  thank you for raising the alarmId column issue for discussion, I somehow completely skipped over that when I initially read the MR07:51
flypigThanks chriadam. I think it'll take too long to digest during the meeting, but I'll read through & comment on the PR.07:51
dcalisteIndeed, let's keep this for next week so we have time to diggest the info and the various possibilities.07:51
dcalisteThank you flypig.07:52
flypigNo, thank you :)07:52
chriadamwas there anything else to discuss today?  flypig and I have another meeting in a few minutes also, so we might have to cut this one a bit short if there are other topics, unfortunately07:53
dcalisteNo that's all. Thank you for your time. I'm also starting a zoom meeting with one of our student. Life is busy !07:54
chriadamjust fyi, there's a bunch of changes (mostly on browser side) which we need to shepherd through CI over the next couple of days, and then branching should happen, so we probably won't get a chance to merge things this week07:54
chriadambut hopefully from next week onwards I should have more time to spend on these ones.  thank you very much again for your ongoing work, and patience with us ;-)07:55
dcalisteNo problem, merging current pending MRs are not in a hurry.07:55
chriadamok! well, have a great week, see you next time!07:55
* chriadam -> away07:55
*** frinring_ is now known as frinring08:21
*** Sellerie2 is now known as Sellerie11:02
*** vilpan_1 is now known as vilpan_11:26
*** juhaj_ is now known as juhaj19:21

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