Tuesday, 2022-04-12

dcalisteGood morning pvuorela, how are you ?07:00
pvuoreladcaliste: good morning, fine thanks.07:00
dcalisteThank you for reviewing https://github.com/sailfishos/mkcal/pull/29 yesterday.07:01
dcalisteI asked you about making friend the SqliteFormat class, but thinking about it over night, it could be too specific.07:02
dcalisteWhat about discoupling internal flags in the object and the flags in the DB ?07:02
pvuorelawas left pondering if the flags belong at all to the notebook or should it be backend specific.07:03
dcalisteThen, 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
dcalisteExactly, it would become backend spoecific doing like that.07:03
pvuorelacould be cleaner.07:03
dcalisteFor backward compatibility, of course, they would be identical at the moment.07:03
pvuorelai 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
dcalisteOk, 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
pvuorelawonder if we could do something for that repeating default notebook api in multiple places.07:06
dcalisteIndeed, but they are dealing with different things (I guess you know, but for sake of clarity):07:06
pvuorelalooks 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
pvuorelaor 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
dcalisteThat's the current situation.07:09
dcalisteThe calendar, even if not used internally by KCalendarCore, it is used in ExtendedCalendar.07:09
dcalisteWe use it for what we have discussed already about the addEvent() without NotebookId argument.07:10
dcalisteIn 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
dcalisteBut this would be too much API changes...07:11
dcalisteThe storage should just be a mirror of the calendar and not the place to have additional information.07:12
dcalisteAnyway, 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
dcalisteBut I'm afraid of breaking too many things here.07:14
pvuorelasounds 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
dcalisteChanging the nemo plugin is quite simple as you mentioned, but I'm a bit afraid of the rest. In case...07:15
dcalisteAnd 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
pvuorelai mean sync plugins shouldn't need it. and neither the birthday calendar stuff.07:16
dcalisteIf we move it one day to ExtendedCalendar, then, it would just be a overload of the KCalendarCore::Calendar::setDefaultNotebook() itself.07:16
dcalisteOk, then to remove the isDefault() and setIsDefault() from the notebook API.07:17
dcalisteI'll add it to the PR and create one for the nemo bindings to go with it.07:17
dcalisteNo 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
pvuoreladcaliste: hey btw, think i'll need to skip the next week's meeting.07:21
dcalisteNo 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
pvuorelaalright :)07:23
dcalisteAbout 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
pvuorelahm, ok. i'll need to still catch up with that PR too.07:30
pvuorelasorry, didn't get too far yet reviewing it :/07:30
dcalisteNo 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
dcalisteMR 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
pvuorelano hurry07:37
dcalisteOk, so lets wait for the next version then.07:37
dcalisteMaybe, 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
dcalisteFahrplan needs then to add an event to the calendar.07:41
dcalisteAt 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
dcalisteAnd it makes it impossible to get to harbour also.07:42
dcalisteWe 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
pvuorelaquite a lot of discussion, but yea, openUrl() is good.07:44
dcalisteBut 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
dcalisteThat leads me to thinking that  com.jolla.calendar.ui.openIcsData() may be allowed for everyone and not restricted to Calendar permission only.07:44
pvuorelawell, openUrl is nicely generic.07:45
dcalisteActually, 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
dcalisteThe problem of openUrl() in an application with ICS data generated on-the-fly is when to delete the file ?07:46
dcalisteAnd where to put it ?07:46
dcalisteThe cache is not readable from the calendar application, and put it into Documents/ will clutter it.07:46
pvuorelayea, the temporary file thing is a good point which we could do something for or at least clarify.07:47
dcalisteHaving 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
dcalisteI'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/507:50
pvuorelawell, 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
dcalisteExactly, 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
dcalisteI'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
dcalisteAnd thanks pvuorela for your suggestions in the forum thread :)07:59
pvuorelacheers :)08:02
pvuorelaand have a nice vacation08:02
poetasterpvuorela, 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
pvuorelapoetaster: hey and there's also this https://github.com/sailfishos/lipstick/blob/master/src/compositor/fileservice.xml13:01
pvuorelapoetaster: not allowed in the permissions but could consider doing that. it's not always gui process that wants to share.13:02
poetasterpvuorela, ah, thanks.13:02
pvuorelaor maybe better say open files instead of share. but anyway.13:03
poetasterpvuorela, 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 while13:03
poetasterpvuorela, doing evil things with files is more or less the point of computing :)13:04
pvuorelasure :)13:04
poetasterah, ok so that's a lipstick dbus service that does the same thing as openUrlExternally. hmm.13:05
poetasterThe curren > 4 build actually does what one wants and reduces complexity, so I think I'll focus on that.13:07
poetasterand the memory leaks all over the place. sigh.13:07
pvuorelasure. just noted this for completeness on the options.13:08
poetasterpvuorela, 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/!