| dcaliste | Good morning pvuorela, how are you ? | 07:00 |
|---|---|---|
| pvuorela | dcaliste: good morning, fine thanks. | 07:00 |
| dcaliste | Thank you for reviewing https://github.com/sailfishos/mkcal/pull/29 yesterday. | 07:01 |
| dcaliste | I asked you about making friend the SqliteFormat class, but thinking about it over night, it could be too specific. | 07:02 |
| dcaliste | What about discoupling internal flags in the object and the flags in the DB ? | 07:02 |
| pvuorela | was left pondering if the flags belong at all to the notebook or should it be backend specific. | 07:03 |
| dcaliste | Then, we can use only the public functions of Notebook class (is* and set*) to create the flags in the DB and reread them. | 07:03 |
| dcaliste | Exactly, it would become backend spoecific doing like that. | 07:03 |
| pvuorela | yep. | 07:03 |
| pvuorela | could be cleaner. | 07:03 |
| dcaliste | For backward compatibility, of course, they would be identical at the moment. | 07:03 |
| pvuorela | i suppose it should be safe just removing the notebook flags from the api. the meaning of that is not public anyways so what can anything but storage do with it. | 07:04 |
| dcaliste | Ok, I'll amend the commit in that direction today. I agree that it would be a good idea to remove the flag part of the public API. | 07:05 |
| pvuorela | wonder if we could do something for that repeating default notebook api in multiple places. | 07:06 |
| dcaliste | Indeed, but they are dealing with different things (I guess you know, but for sake of clarity): | 07:06 |
| pvuorela | looks like the Calendar::setDefaultNotebook() is not even used on nemo calendar, which is probably the only place needing the default info. | 07:06 |
| dcaliste | - calendar is from KCalendarCore and it is the root place, I would say, | 07:07 |
| pvuorela | or wait, the calendar one is set by mkcal. | 07:07 |
| dcaliste | - notebook is for a single notebook, then when you have a notebook list you know if it is set default or not, | 07:08 |
| dcaliste | - storage, a convenience API to unset the old default and set the new in one call. | 07:08 |
| dcaliste | That's the current situation. | 07:09 |
| dcaliste | The calendar, even if not used internally by KCalendarCore, it is used in ExtendedCalendar. | 07:09 |
| dcaliste | We use it for what we have discussed already about the addEvent() without NotebookId argument. | 07:10 |
| dcaliste | In my opinion, it is the place to put this default information. Particularly because the notebook listing itself should be part of ExtrendedCalendar and not ExtendedStorage. | 07:11 |
| dcaliste | But this would be too much API changes... | 07:11 |
| dcaliste | The storage should just be a mirror of the calendar and not the place to have additional information. | 07:12 |
| dcaliste | Anyway, the Notebook default API (isDefault() and setIsDefault()) in my opinion, could be removed. The default notebook is a property of the notebook list and not of a single notebook. | 07:13 |
| dcaliste | But I'm afraid of breaking too many things here. | 07:14 |
| pvuorela | sounds sensible. i wouldn't worry too much on api changes here. likely only the nemo plugin is interested about the default and there only in the worker thread. | 07:14 |
| dcaliste | Changing the nemo plugin is quite simple as you mentioned, but I'm a bit afraid of the rest. In case... | 07:15 |
| dcaliste | And to finish with the API review, having setDefaultNotebook() in storage, make sense at the moment, as long as the notebook listing is kept there. | 07:15 |
| pvuorela | i mean sync plugins shouldn't need it. and neither the birthday calendar stuff. | 07:16 |
| dcaliste | If we move it one day to ExtendedCalendar, then, it would just be a overload of the KCalendarCore::Calendar::setDefaultNotebook() itself. | 07:16 |
| dcaliste | Ok, then to remove the isDefault() and setIsDefault() from the notebook API. | 07:17 |
| dcaliste | I'll add it to the PR and create one for the nemo bindings to go with it. | 07:17 |
| pvuorela | thanks. | 07:18 |
| dcaliste | No problem, this setDefaultNotebook() as done at the moment was creating issues when moving to thread handling, because of numerous calls to the thread worker for one action. That's why I ended up dealing with it. | 07:19 |
| pvuorela | dcaliste: hey btw, think i'll need to skip the next week's meeting. | 07:21 |
| dcaliste | No problem, I was going to say that I'll be off for three weeks myself ! I'll be on vacations next week up to the Next Tuesday and be off again the first week of May. | 07:23 |
| pvuorela | alright :) | 07:23 |
| dcaliste | About the PR preparing thread implementation, I ended up with the following : | 07:26 |
| dcaliste | - StorageBackend and thus SqliteStorage are now only dealing with reference on read and pointers on creation. | 07:27 |
| dcaliste | - Code using a STorageBackend, like ExtendedStorage, are then required to implement a StorageBackend::Manager to get ownership of the created pointers (notebooks or incidences). | 07:28 |
| dcaliste | - the in-thread implementation (current situation) is simply taking ownership with QSharedPointer(), while the threaded implementation (not yet published) is passing the memory pointers to the main thread in a blocking signal emission so the main thread can take ownership itself with QSharedPointer(). | 07:29 |
| pvuorela | hm, ok. i'll need to still catch up with that PR too. | 07:30 |
| pvuorela | sorry, didn't get too far yet reviewing it :/ | 07:30 |
| dcaliste | No problem, I was just taking the occasion of this little chat to explain the idea of the changes, so it can be easier for you to review when you will have time. | 07:32 |
| pvuorela | sure | 07:32 |
| dcaliste | MR about missing UIDs in KCalendarCore has been accepted yesterday. I'm not going to make a PR yet, waiting to see if some other changes will come. Except if branching to 4.5 is due soon. In that case, it would be better to get this functionality in. What do you think ? | 07:37 |
| pvuorela | no hurry | 07:37 |
| dcaliste | Ok, so lets wait for the next version then. | 07:37 |
| dcaliste | Maybe, one last word : https://forum.sailfishos.org/t/nemo-qml-plugin-calendar-timeline-for-harbour/11119 I discussed with poetaster about updating Fahrplan to the latest KCalendarCore API. | 07:40 |
| dcaliste | Fahrplan needs then to add an event to the calendar. | 07:41 |
| dcaliste | At the moment, it is doing it by direct mKCal calls, which is not ideal (a mistake and the wall database is messed up). | 07:42 |
| dcaliste | And it makes it impossible to get to harbour also. | 07:42 |
| dcaliste | We ended up using QDesktopServices::openUrl() with ICS data generated by the app, which works more or less fine and should be harbour compatible. | 07:42 |
| pvuorela | quite a lot of discussion, but yea, openUrl() is good. | 07:44 |
| dcaliste | But the best way in my opinion would be to directly use com.jolla.calendar.ui.openIcsData() instead of passing by a temporary file (which we don't know when to delete because it must be present up to the dialog acceptence in the calendar app). | 07:44 |
| dcaliste | That leads me to thinking that com.jolla.calendar.ui.openIcsData() may be allowed for everyone and not restricted to Calendar permission only. | 07:44 |
| pvuorela | well, openUrl is nicely generic. | 07:45 |
| dcaliste | Actually, Calendar permission is giving full read access to the DB (and write also), while we just want to contact the calendar application so it can propose the open dialog. This is much safer than giving Calendar permission. | 07:46 |
| dcaliste | The problem of openUrl() in an application with ICS data generated on-the-fly is when to delete the file ? | 07:46 |
| dcaliste | And where to put it ? | 07:46 |
| dcaliste | The cache is not readable from the calendar application, and put it into Documents/ will clutter it. | 07:46 |
| pvuorela | yea, the temporary file thing is a good point which we could do something for or at least clarify. | 07:47 |
| dcaliste | Having the DBus call (and only this one) allowed for anyone would not create security break, the user will always be the last to decide with the dialog to accept or reject the calendar addition. | 07:48 |
| dcaliste | I'm thinking about creatring a PR in sailjail-permissions for this so arguments pro and con can be discussed there. Do you think it's worth it ? It can be in relation also to poetaster question for next Thursday community meeting : https://forum.sailfishos.org/t/community-meeting-on-irc-14th-april-2022/10931/5 | 07:50 |
| pvuorela | well, committing to dbus api is always one thing. earlier if one used such it was on their own risk, but api explicitly on permissions it gets a bit more advertised. | 07:51 |
| dcaliste | Exactly, that's the con part I guess : it creates a kind of stability promise for this function from UI part... Well if you don't think it's useless, I'll create the PR in permissions and we can discuss there the advantages and disadvantages. | 07:53 |
| dcaliste | I'll have to go. Thank you pvuorela for the discussion today. I'll update the mKCal PR about default notebook. See you May 10th or earlier in a PR discussion. | 07:57 |
| dcaliste | And thanks pvuorela for your suggestions in the forum thread :) | 07:59 |
| pvuorela | cheers :) | 08:02 |
| pvuorela | and have a nice vacation | 08:02 |
| poetaster | pvuorela, I agree with dcaliste that dbus is the better way. openurl 'works', but if you're in a thread out of gui context, it's not really workable. | 08:39 |
| poetaster | https://github.com/poetaster/fahrplan/blob/25f77fd875c34aaa36f956e6309a6eae97ca944c/src/calendarthreadwrapper.cpp#L209 | 08:41 |
| pvuorela | poetaster: hey and there's also this https://github.com/sailfishos/lipstick/blob/master/src/compositor/fileservice.xml | 13:01 |
| pvuorela | poetaster: not allowed in the permissions but could consider doing that. it's not always gui process that wants to share. | 13:02 |
| poetaster | pvuorela, ah, thanks. | 13:02 |
| pvuorela | or maybe better say open files instead of share. but anyway. | 13:03 |
| poetaster | pvuorela, my aim with fahrplan is to get it back into harbour, simplify the build and with a bit of luck support 3.x for a while | 13:03 |
| poetaster | pvuorela, doing evil things with files is more or less the point of computing :) | 13:04 |
| pvuorela | sure :) | 13:04 |
| poetaster | ah, ok so that's a lipstick dbus service that does the same thing as openUrlExternally. hmm. | 13:05 |
| pvuorela | yea | 13:05 |
| poetaster | The curren > 4 build actually does what one wants and reduces complexity, so I think I'll focus on that. | 13:07 |
| poetaster | and the memory leaks all over the place. sigh. | 13:07 |
| pvuorela | sure. just noted this for completeness on the options. | 13:08 |
| poetaster | thanks! | 13:08 |
| poetaster | pvuorela, I was wondering why default calendar is 'none' | 13:14 |
| HengYeDev[m] | is there a signal for qml webview when the url changes? | 22:32 |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!