| dcaliste | Hello chriadam. I hope you're well. | 07:01 |
|---|---|---|
| chriadam | hi dcaliste, yes I am doing well thanks. how has your week been? | 07:02 |
| dcaliste | It 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 |
| dcaliste | I need to update the MR to actually use your modifications instead of my quick compilation fix. | 07:04 |
| dcaliste | see https://git.sailfishos.org/mer-core/messagingframework/merge_requests/59 | 07:05 |
| chriadam | I 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 check | 07:05 |
| chriadam | am 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 |
| dcaliste | Monich's patch upstream is https://codereview.qt-project.org/c/qt-labs/messagingframework/+/308665 | 07:06 |
| dcaliste | The Qt6 move was required to be able to upstream patches that were pilling up in git.sailfishos.org. | 07:07 |
| chriadam | oh 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 think | 07:07 |
| dcaliste | I'm not completely sure that it will help any Qt transition to something strictly lower than 6.0 though. | 07:07 |
| dcaliste | Because most modifications were just deprecations in Qt5.xx. | 07:08 |
| chriadam | true | 07:08 |
| chriadam | so 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 |
| dcaliste | That being said, I was suprised to the actual low neumber of revert patches that was needed to compile in SDK. | 07:09 |
| dcaliste | Yes, 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 |
| dcaliste | It would avoid issues like the problematic rebase of monich's patch. | 07:10 |
| dcaliste | Because 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 |
| chriadam | right, makes sense. I've added a comment to the MR for context | 07:11 |
| chriadam | that one will probably not be merged before we move to github, I guess, so might have to recreate the MR there shortly | 07:11 |
| chriadam | pvuorela: ^ | 07:11 |
| dcaliste | No problem to recreate the MR in github when necessary. It's just a matter of push plus some copy-pasting. | 07:12 |
| chriadam | indeed, thanks | 07:13 |
| chriadam | going on from last week's topics: | 07:14 |
| dcaliste | I'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 |
| chriadam | 1) mkcal MR#63 has some discussion from flypig, my understanding is that he agrees that the patch shouldn't cause issues on EAS side | 07:15 |
| chriadam | 2) mkcal MR#65 has some comments from Pekka, I saw. I think adding proper support for attachments would be nice. | 07:17 |
| chriadam | I guess that one is blocking nqpc#86 currently | 07:19 |
| dcaliste | About 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 |
| chriadam | 3) 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 |
| chriadam | yup, I will poke Pekka about (1) and maybe get that in, during the next day or two I hope. | 07:21 |
| flypig | For 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 |
| chriadam | flypig: true. maybe we should wait until after 4.2.0 branching | 07:21 |
| chriadam | before merging | 07:21 |
| flypig | That sounds safer, although I guess there's still a branching -> releasing gap to test it in. | 07:22 |
| dcaliste | As you think is better. Waiting after branching causes no problem, since the target of this is only code simplication (hopefully). | 07:23 |
| chriadam | yeah. my first (risk averse) instinct is to wait, and merge the change next week instead. | 07:24 |
| dcaliste | The 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 |
| dcaliste | When you look at the IncidenceHandler::incidenceToExport() function, we're not far. | 07:25 |
| dcaliste | There 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 |
| dcaliste | And 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 |
| chriadam | I wonder if we can fix that by making a combination of UID+notebookUid the primary key / uniqueness contraint | 07:28 |
| chriadam | in mkcal | 07:28 |
| chriadam | of course, will then require some migration of existing data step | 07:28 |
| dcaliste | And 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 :^D | 07:29 |
| dcaliste | For the UNIQUE constraint, yes it could be eased by migrating the database to add the notebookid on the constraint. | 07:29 |
| dcaliste | But 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 |
| chriadam | indeed | 07:31 |
| dcaliste | About your point 2), I've seen the comments from pvuorela (thanks for them) and will address them later today. | 07:32 |
| dcaliste | There is one point that we may discuss here though, which is the support for attachment for alarms. | 07:33 |
| dcaliste | The RFC says that ATTACH can be present for VEVENTs, VTODOs and VJOURNALs. Which is covered by the MR. | 07:33 |
| dcaliste | But it can also be added for VALARMs. Which is not covered at the moment. | 07:36 |
| chriadam | do we support importing VALARMs currently? | 07:39 |
| chriadam | I wasn't aware we even supported importing VTODO or VJOURNAL entries honestly | 07:39 |
| chriadam | so, this doesn't sound like a problem to me | 07:40 |
| chriadam | or is there something I'm overlooking? | 07:40 |
| dcaliste | The idea is for this MR to be future compatible. | 07:40 |
| dcaliste | See my latest comment in this MR. | 07:40 |
| chriadam | ah, the alarmId column | 07:42 |
| dcaliste | Indeed. 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 |
| dcaliste | With enough comment in the code it could not be that bad. | 07:44 |
| chriadam | oh, is componentId not part of a compound key? | 07:44 |
| dcaliste | No, the layout of the database is very crude and all cross table is done by hand in the C++ code. | 07:46 |
| chriadam | unsurprising, I suppose | 07:47 |
| chriadam | well, 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 |
| dcaliste | You have better knowledge on this than myself. If it looks to surprising then it's not a good solution. | 07:49 |
| flypig | Sorry, 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 |
| dcaliste | And particularly this comment : https://git.sailfishos.org/mer-core/mkcal/merge_requests/65#note_88470 | 07:51 |
| chriadam | well, 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 MR | 07:51 |
| flypig | Thanks chriadam. I think it'll take too long to digest during the meeting, but I'll read through & comment on the PR. | 07:51 |
| dcaliste | Indeed, let's keep this for next week so we have time to diggest the info and the various possibilities. | 07:51 |
| chriadam | tyvm | 07:51 |
| dcaliste | Thank you flypig. | 07:52 |
| flypig | No, thank you :) | 07:52 |
| chriadam | was 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, unfortunately | 07:53 |
| dcaliste | No 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 |
| chriadam | just 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 week | 07:54 |
| chriadam | but 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 |
| dcaliste | No problem, merging current pending MRs are not in a hurry. | 07:55 |
| chriadam | :-) | 07:55 |
| chriadam | ok! well, have a great week, see you next time! | 07:55 |
| * chriadam -> away | 07:55 | |
| *** frinring_ is now known as frinring | 08:21 | |
| *** Sellerie2 is now known as Sellerie | 11:02 | |
| *** vilpan_1 is now known as vilpan_ | 11:26 | |
| *** juhaj_ is now known as juhaj | 19:21 | |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!