Tuesday, 2020-03-03

*** zbenjamin_ is now known as zbenjamin02:26
dcalisteHello chriadam, I hope you spent pleasant holidays.08:06
chriadamhi dcaliste, yes it was a nice vacation, thanks!08:07
chriadamI hope your February was pleasant also08:07
chriadamI saw that you have been implementing timezone selector support for events - thanks for that!  I haven't had a chance to review that PR but I saw Pekka has been reviewing that one anyway08:09
dcalisteYeh, it was alright. If we look at what was left before your vacations, not much in fact : mainly kcalcore!1508:09
chriadamI will check that08:09
dcalisteAbout the timezone, yes, now that I understand better how mkcal is handling those, I was a bit more confident to allow it in UI.08:10
dcalisteThere are few additions in nemo-qml-plugin-calendar to export the time spec and time zone.08:10
dcalisteAnd following pvuorela advice, I added to entries in the time zone picker (none and UTC) and added a value button in EventEditPage.qml08:11
chriadamexcellent, sounds like that one is progressing very nicely, thanks for that08:11
dcalisteThe modifications in nemo-qml-plugin-calendar are worth discussing though08:11
dcalisteIn fact there is currently an inconsistency in the event edit page:08:11
chriadamkcalcore!15 looks good to me, I will merge that one later this week if no-one else complains (and my on-device tests don't show issues)08:11
dcaliste(thanks for kcalcore!15)08:12
dcalisteIf you edit an event that was created in a time zone different from the current one, the time are shown in the origin time zone, but...08:12
dcalisteif the event is UTC, the time in the current time zone is shown and if the event is an occurrence of a recurring one, the time in the current time zone also is shown.08:13
dcalisteThis is due to the fact that:08:13
dcaliste- occurrence are treated differently from event. The occurrences are using QDateTime, with a transformation from KDateTime using toLocalZone().dateTime()08:14
dcaliste- the same for UTC, the transformation dateTime() (without the call to tolocalZone()) is treated specifically in KDateTime, and the QDateTime is returned as UTC, which is then converted to local time during the QDateTime to JS transformation.08:15
dcaliste- while for plain event, the KDateTime.dateTime() is used without the toLocalZone().08:16
dcaliste(see calendarevent.cpp and calendareventoccurrence.cpp)08:16
dcalisteThis is inconsistent.08:17
chriadamfor the first case (edit event created in different time zone) is it shown in the UI which timezone it is in?  i.e. via the combobox you are adding to the UI?08:17
dcalisteI'm speaking at the moment without my modifications.08:17
dcalisteThese are the current state of calendar UI.08:17
dcalisteMy point is :08:18
dcaliste- it should good to use the toLocalZone() conversion for the plain events also, but08:18
dcaliste- this clash with my proposed modifications, because having the time in the original time zone (or UTC) is nicer when editing, specifically if the time zone is shown and can be changed.08:19
dcalisteSo two possibilities:08:19
dcaliste- patch the current state for the plain event to be edited in current time zone,08:20
dcaliste- or apply my patches ;)08:20
dcalisteThe second possibilities, requires that UTC and occurrences can return the time in the parent time zone.08:21
dcalisteIf you look at my patch it is done in:08:21
dcaliste- https://git.sailfishos.org/mer-core/nemo-qml-plugin-calendar/merge_requests/51/diffs#fc0210dcd0d26893492d1d1a3b666c4607678df9_68_6708:21
chriadamwhen you say "plain event" here, you mean 1) not an occurrence, 2) start time not specified in UTC ?  so you want to call .toLocalZone() prior to display in UI?  (but that would conflict with your change, since we want to show the actual tz and allow user to specify it)?08:21
dcalistefor the UTC part.08:21
dcalisteYou're right, that's my point.08:22
dcalisteIt's conflicting.08:22
dcalisteThere are two possibilities to solve the inconsistency, but they are mutually exclusive.08:22
dcalisteI'm in favour of my patch and solution of course, but it's fair to expose the other one also.08:23
chriadamok, I think I'm understanding up to that point.  what I'm confused about is: how does your MR#51 address this issue? (I haven't yet looked at it in detail, or forgot)08:23
chriadamdoes it ensure that the timezone info is exposed in the event and occurrence data to QML?08:23
chriadamand then always show the start date etc in the appropriate timespec for that timezone?08:24
dcalisteYes, for UTC fix, follow the link above, and for the occurrences, it is...08:24
dcalisteFor UTC, it is simple, just remove the spec info and create a QDateTime (local zone) with the time of the event in it's zone.08:25
dcalisteFor occurrence, one need to move the QDateTime of the occurrence to the parent time zone.08:26
dcalisteAnd export it to QML via another property than startTime (which is in local zone).08:26
*** frinring_ is now known as frinring08:27
dcalisteThe name I choose for the new property is _really_ bad, but I had no better idea.08:27
dcalisteoccurrence has two properties now :08:27
dcaliste- startTime, the time in local zone (historical, and inconsistent with event.startTime which is in origin time zone)08:27
chriadamwhat do you mean "to the parent time zone" (I'm wondering about the case where the occurrence has its own custom dtStart, i.e. a persistent exception - or is that covered by other event type, and only "non-exception" events are exposed as occurrences?)08:28
dcaliste- startDateTime, my new property in the origin time zone.08:28
dcalisteoccurrences in nemo-qml-plugin-calendar, as far as I undestand, has a slightly different meaning than occurrences in mkcal.08:28
chriadampvuorela: any thoughts on this?  I wonder what compat we promise here, maybe we could make occurrence.startTime consistent with event.startTime i.e. perform the appropriate tiemzone transform rather than localtz08:29
dcalisteThey are events (or exceptions, or recuurring occurrences) at a given local time to be shown in the calendar. And they refer to an event. For simple plain event, there is one occurrence for one event.08:30
dcalisteFor recurring ones, they are plenty, including exceptions.08:30
chriadamI see08:30
dcalisteI though about changing the meaning of occurrence.startTime to be consistent, but I'm afraid of unexpected consequences.08:31
dcalisteTo come back to occurrences, they are always given in local zone, that's why displaying an event in UI is showing the local time what ever the time zone it was entered in, because EventView.qml is using the occurrences atratTime.08:32
dcalisteWhich is fine.08:32
chriadamindeed.  maybe pvuorela or flypig have some comments there, I guess it's only jolla-calendar which uses nemo-qml-plugin-calendar currently?  not sure...08:32
dcalisteOnly the editing page is using the event.startTime for plain events and occurrence.startTime for specific occurrence of a recurring series.08:33
dcalisteTo summarize, in my opinion:08:33
dcaliste- make UTC event.startTime give the time in UTC to be consistent with event.startTime being in its time zone;08:34
dcaliste- add a new property in eventoccurrence to expose the time in the event time zone (if any), but keep occurrence.startTime in local zone not to break anything.08:35
dcaliste- display the time zone (or UTC or none) in UI, so everything is consistent.08:35
dcaliste(find a better name for the new property in eventoccurrence)08:36
chriadamI agree with everything, except that I wonder about the "keep occurrence.startTime" -- I guess I don't fully understand why the "old" one would still be used, and maybe we could just change the semantics of the property.  but maybe I'm misunderstanding something.08:37
dcalisteFor editing it's nice to have the original time, but for viewing (in the event list, or in the event page), we need the time to be in the local zone, always. So occurrence.startTime in local zone is still required.)08:38
chriadamaaah I see08:38
dcalisteThat's also why, having event.startTime and eventoccurrence.startTime being not in the same time reference makes sense also.08:39
dcalisteA bit sad that the name are identical, that can infer confusion.08:39
dcaliste(it took me two days to see the point of separating event and eventoccurrences).08:39
chriadamsure.  well.  I agree, let's have some more thought about the property name.  aside from that, in the MR you mention that you want to add some unit tests?08:42
dcalisteYes, I looked quickly at the existing test and wanted to add some more, but it was not clear to me if it was going to scratch my mkcal storage if I run the test on device. And this I don't want ;) So at the moment, I didn't add any. But I would like to.08:43
chriadamhrm, good question :-(08:43
chriadamI think there is a test flag, it creates a db in the cwd08:44
chriadambut not sure whether the nqpc tests make use of that etc08:44
dcalisteNormally using SQLITESTRAGEDB=foo, it creates a local db, at least for mkcal, that's what I'm doing. For buteo-sync-plugin-caldav also.08:45
dcalisteSo I guess I can do the same here. But didn't look deep enough yet to convinced myself.08:45
chriadamyep, no worries.  thanks for the explanations and for your work also.  I will ask Pekka and David to take a look also, but my opinion is that your PRs are the right way to go.08:47
dcalisteThanks. It required to be discussed indeed. The current state was not completely clear and this local time / another time zone can quickly become complex.08:47
chriadamI had nothing else to discuss - mostly still just trying to catch up on emails, bugs, etc things from while I was away on vacation, so didn't progress anything yet08:49
dcalisteBefore concluding the meeting, there are two things that I would like to quickly discuss :08:49
dcaliste- https://git.sailfishos.org/mer-core/buteo-syncfw/merge_requests/4508:49
dcaliste(very simple one, not important at all, about writing the schedule status in log file for some error cases)08:50
dcaliste- https://git.sailfishos.org/mer-core/libcommhistory/merge_requests/3608:50
dcaliste(adding a way to export comm history for unregistered number)08:50
chriadamblam and Venemo know more about commhistory than I do at this point08:51
VenemoI usually just hit it with a hammer until it starts doing what I want08:52
dcalisteVenemo, yeh, that's the problem. The point here is how to allow to reuse the code from the singlecontactevent for unsaved numbers.08:53
chriadamdcaliste: that feature (showing commhistory of non-saved contact) is super important IMO.  I will poke mschuele about it.  unfortunately jpetrell is away on paternity leave for a while, but hopefully pvuorela and mschuele will push that one forward08:53
dcalistepvuorela notices that using inheritance as I'm proposing is not very elegant. Which I agree, it has some drawbacks.08:53
VenemoI can't give you a good answer dcaliste without reading the code myself08:54
dcalisteSo guys, if you have an idea (after reading the code of course), please comment in the MR.08:54
Venemobut I can do this if you feel that it's important08:54
dcalisteI put the link again here : https://git.sailfishos.org/mer-core/libcommhistory/merge_requests/3608:55
Venemoyeah, already clicked08:55
chriadamdcaliste: I cannot promise to look at it this week, unfortunately, but I will poke people.  I think the use-case is an important one.  can discuss again next week and I can allocate more time to it.08:55
chriadamthanks Venemo08:55
Venemothough I don't exactly see what this is set to accomplish, I can already see the unsaved numbers in my phone app08:56
dcalisteI know it's not always a very good reason, but the changes to have it working in UI is quite minimal when allowing the QML contactEventModel to fallback on a remoteId as done here.08:56
dcalisteTap on an unsaved number to open the card.08:57
dcalisteAt the moment, you only have the number, but no history, like for a contact.08:57
Venemoif I tap it it just calls it again08:58
dcalisteWith the patch, you can also see comm history for unsaved numbers, and see when you where called and hoa many times.08:58
Venemoah, I see.08:58
dcalisteAh, yeh, I use the option "tap to view the contact card, don't call back".08:58
Venemoyou mean you have "quick call" turned off08:58
VenemoI think then the call ui would need to be patched to add "show contact card" on unsaved numbers, too, to bring it up to parity08:59
Venemobut yeah, I think I get it now08:59
dcalisteGood point, I can see to add this indeed. Thanks for pointing it to me.08:59
Venemothough it's also a good question if the term "contact card" is something that makes sense for an unsaved contact09:00
dcalisteSure, can think of a better wording for unsaved numbers.09:00
Venemodunno. it is a contact card, after all.09:01
Venemochriadam: any idea?09:01
chriadamI think contact card is fine nomenclature, personally, but haven't given it much thought09:01
chriadamI agree that some "show contact card" action is needed for such unsaved contacts09:01
chriadamthe main concern I have is about the implementation, as per Pekka's comments09:02
chriadambut that can be iterated / discussed09:02
Venemoyeah, sure09:04
chriadamdcaliste: thanks very much for working on that!  much appreciated.09:04
chriadamwas there anything else to discuss?09:04
flypigCan I also please ask a question of you guys about calendars?09:04
dcalisteOk, that's all for today. Thanks everyone for the feedback and discussions.09:04
Venemoalso, libcommhistory is some of the ugliest code I've ever seen, so I don't think we should be too pedantic with dcaliste to be honest09:04
chriadamflypig: go for it09:04
dcalisteflypig: sure09:05
flypigThanks :) I'm looking at the issue of calendars remaining in the calendar app even when they're disabled.09:05
dcalisteflypig: do you mean the account is disabled ?09:05
flypigYes, exactly. For example, disabled from the Settings app (long press on the account icon > disable).09:06
chriadam(btw I think that might be intentional... at least, it used to be intentional that disabling the account basically just disabled sync, but didn't delete data (photos etc) which had already been synced)09:06
chriadambut maybe it's time to revisit that decision09:06
dcalisteVenemo, thanks, but no problem discussing contributions, it's always fruitful and I often learn things.09:06
flypigIt I'm not sure whether it's intentional, but it's at least inconsistent, because it does it for EAS.09:06
Venemodcaliste: yeah, it's just the feeling I get when I look at libcommhistory :P09:07
flypigIt doesn't seem to delete the data, just doesn't show it any more.09:07
chriadamflypig: that would be the ideal solution, IMO.  hide without deleting.09:07
flypigAnd that's different from disablying sync, which leaves the entries there (just to clarify the terminology).09:07
flypigchriadam, yes, I agree.09:07
flypigIt doesn't look so hard to achieve, because notebooks can be disabled in mkcal. This is how EAS does it, I think.09:08
dcalisteflypig, I think the pipe work is in buteo msync/AccountsHelper.cpp09:08
dcalisteLet me give a look...09:08
chriadamflypig: of course, it gets more complex for other datatypes like images, since I didn't consider this when creating the schemas for that ;-)09:08
flypigCould you elaborate please chriadam?09:09
chriadamflypig: if you want to fix it for calendars, I hope you also fix it for contacts, images, eventsview entries, etc ;-P09:09
flypigdcaliste, that's really my question. It seems to work for EAS because there's a daemon to notice account changes. Ideally there would be a daemon to do it for calendars too, but I'm not sure buteo is the corecct place.09:10
dcalisteflypig, ah no, it's not in AccountsHelper.cpp, it is only responsible for data deletion on account deletetion...09:10
chriadamflypig: I mean we created a bunch of databases (like libsocialcache etc) to store metadata for images, and I didn't add a "accountHidden" flag in its schema to allow the UI to hide entries appropriately09:10
chriadamsimilarly, for contacts, we would need some "accountHidden" flag associated with each contact or whatever, and not return it in filter requests etc09:10
chriadamfor consistency09:10
chriadamwith hidden calendars09:11
flypigYes, I see what you mean. I guess it's worth me adding that it's a particular issue for calendars, because alarms continue to trigger on disabled calendars.09:11
flypigAs it happens, hiding calendar entries won't fix the problem, but the two do seem to conceptually go together.09:12
dcalisteflypig, I think it's mkcal which should disable alarms on disabled notebooks.09:13
chriadamI agree.  it doesn't make sense to hide calendar entries but still play the alarms, and it wouldn't make sense to NOT hide the calendar entries, but not have them alarm you when required.09:13
flypigchriadam, yes, exactly.09:13
chriadamdcaliste: right, mkcal has some link to timed iirc09:13
flypigdcaliste, yes, that makes sense, but I don't think it happens right now.09:13
dcalisteBut I'm pretty sure mkcal is not doing it at the moment, just doing it when deleting entries.09:14
flypigdcaliste, correct: EAS disbles mkcal and hides events, but alarms continue to go.09:14
flypigwhereas other accounts do neither.09:14
flypigi.e. other accounts continue to show and alarms continue to trigger.09:15
dcalistedding the code in mkcal to stop alarms for disabled events and disbaled notebooks is then a first step.09:15
chriadamflypig: mkcal ExtendedStorage::clearAlarms(const QString &notebookUid)09:16
flypigdcaliste, yes, although I'm approaching it from the other side, trying to figure out where to disable mkcal events.09:16
flypigchriadam, dcaliste, great, that looks good :)09:16
dcalisteWhat do you mean disable mkcal events ?09:16
chriadamhe means "mkcal-sourced timed events" I think09:16
flypigdcaliste: there needs to be a process that notices when an account is disabled, and disables the mkcal notebook.09:17
flypig(and also the reverse).09:17
chriadamflypig: ah.09:17
dcalisteflypig, yes, that's my second step ;)09:17
chriadampreviously, we stuck that kind of glue into contactsd09:17
chriadamjust because it exists09:17
*** Renault_ is now known as Renault09:18
chriadamin the mystical future when we have daemonised system APIs (e.g. offer system apis via IPC), the calendar service would do that09:18
flypigchriadam, yes, so that would be a possibility. I was thinking of creating a new simple daemon for the process, but if there's somewhere it can go alrady?09:18
dcalisteBut I have no idea just now where to put this second step of mine.09:18
chriadamI would put it into contactsd, personally09:18
flypigThe options seem to be: contactsd, buteo, something new.09:19
chriadamnot buteo09:20
chriadamI think so.  it's not a sync plugin, and buteo plugins should be sync plugins IMO09:20
dcalisteButeo is out of the game because it  does not know that an account is to handle calendars. It just knows that an account has a profile for syncing, using a plugin called caldav for instance.09:20
flypigOkay, well I agree, it doesn't seem right. And if it's technically troublesome too, then it makes sense to rule it out.09:21
dcalisteAnd it can call the plugin. But adding a new interface in Buteo for something that is not related to sync is not appropriated in my opinion.09:21
chriadamas I said, if there were a calendard I'd put it in that, but there isn't.  contactsd exists, and already interacts with calendar in the birthdays plugin etc.09:21
chriadamthere will come a time when I will take a large axe to contactsd and remove all of the telepathy/jabber stuff09:21
chriadambut its other functions (watching for changes and triggering appropriate sync profiles; birthdays plugin) remain viable.09:22
flypigOkay, so I'll look at contactsd and try to fit it in there then?09:22
chriadamthat'd be my first suggestion, yes.09:22
flypigOkay, great, thanks, that's very helpful.09:22
chriadambut feel free to ignore my suggestion09:22
dcalisteI'm feeling bad saying this, but I think the code may go to the proprietary part handling accounts, just where the account is disbaled.09:22
flypigWell, I'd rather use something already there if it makes sense, but I'd prefer not to discover it's a bad place after having made the changes, which is why I wanted to check!09:23
chriadamdcaliste: that is one option (i.e. in the UI code which does the disablement, call some service to say "do the account disablement stuff for its sync services")09:23
dcalisteThere is can call mkcal directly like I did in jolla-calendar to get the account id from notebooks.09:23
chriadamthe other option is to set up an AccountManager to listen for enabledChanged signals and react appropriately.  not sure which is better.  but in either case, the actual processing can be done in opensource place like contactsd I think09:24
flypigSorry, I lost the line of thought here.09:25
flypigAre there are two new options here?09:25
chriadamflypig: I think dcaliste is saying: instead of having some service which "listens" for account->disabled changes, you could do it imperatively at the point where the user disables the account via the UI09:25
flypigAre yes, I see.09:25
flypigI looked at that, but it seems problematic with the current implementation.09:26
chriadammy concern with that is: MDM09:26
flypigYes, that's also a good point.09:26
flypigIdeally, it would happen wherever the account is disabled from.09:26
dcalisteWhich may no be a good idea neither since accounts may be disabled outside the UI...09:26
flypigBut when an account is disabled, there's generic code, and no hook to add anything specific.09:26
chriadamright.  I think contactsd listener makes sense.  give it a crack and see how far you get ;-)09:26
dcalisteI don't know contactsd, so if chriadam thinks it's a good place, that's fine with me.09:27
chriadamit's... awful of course.  but it exists ;-)09:27
dcalisteIf you need help for the mkcal part to remove alarms of disabled events, tell me.09:27
flypigGreat, thanks again. dcaliste concerning the alarm disablement, I've not got on to that yet, but if you have further ideas, I'd of course be interested.09:27
flypigOh, sorry, crossed messages. Thank you dcaliste!09:27
chriadamshould hopefully just be a call to that mkcal ExtendedStorage method09:28
dcalistechriadam, I agree.09:28
chriadamafter looking up the notebooks associated with the given account id09:28
flypigOkay, that sounds relatively straightforward. If I hit issues, I'll get back to you for sure.09:28
chriadamsounds good :-)09:29
chriadamok, anythign else to discuss?  if not, thanks all, and have a great week!09:29
dcalistechriadam, have a good evening, see you later.09:29
flypigThank you again!09:29
dcalisteflypig: in extendedStorage, the updateNotebook() method is the only one used to change attributes of a notebook, so a call can be added there to disable alarms if the notebook is disabled, or reverse.09:30
flypigdcaliste, I'll take a look. I know you have many things to work on, but do feel free to create a PR for it you already have the approach in mind. Otherwise I'll probably start working on this part on Friday (earliest).09:34
dcalisteOk, I will try to create a MR for that before the end of the week.09:35
flypigdcaliste, or if you start looking at it but don't have anything ready by friday, just let me know so we don't duplicate effort, please :)09:41
dcalisteflypig, do you have some time to discuss the notebook visible issue in mkcal and friends ?13:43
flypigdcaliste, sure.13:44
flypigWhat were your thoughts?13:44
dcalisteI'm looking at changing the alarm hooking in mkcal depending on notebook visibility.13:44
flypigOkay, great, sounds good.13:44
dcalisteWhich consequently, bring me to how UI is handling that in calendar setting page ?13:44
flypigHow UI changes enabled/disabled status?13:45
dcalisteIt is using a excludedNotebook string array that is passed to nemo-qml-plugin-calendar.13:45
flypigOne second: which account type are you looking at, and which option?13:45
dcalisteThen, inside the calendarworker, ::eventOccurrences() is filtering out events returned by mkcal depending on this excluded list.13:46
dcalisteI'm looking at code in ui-jolla-calendar/application/pages/SettingPages.qml13:46
dcalisteThe calendar setting page in the calendar app, to switch notebook visibility.13:46
dcalisteToggling the switch there then is changing the excludedNotebook property of the CalendarManager from nemo-qml-plugin-calendar.13:47
flypigAh yes, okay, now I understand... I'm wondering if that's how it's done with EAS though. that may be different.13:47
flypigBut anyway, I'm with you now.13:47
dcalisteOk, then…13:48
dcalisteIncalendarworker.cpp::eventOccurrences() the event list returned by mkcal is filtered out to remove occurrences from excluded notebooks.13:49
flypigYes, I see the code.13:50
dcalisteMy first question (you may not being able to answer, but I would like to raise the question) is why not using the Notebook::isVisible() property from mkcal and dupplicating here in a QSetting the excluded property for notebooks in the calendar worker ?13:50
dcalisteBecause if I want to hook something in mkcal on notebook visibility, this attribute should be present at mkcal level, which is not the case at the moment…13:51
flypigEAS seems to do something more in line with what you're suggesting:13:52
flypig        notebook->setIsVisible(visibleState);13:52
dcalisteIs it clear what I mean ? nemo-qml-plugin-calendar is using its own way to set notebook visibility while mkcal is providing the same functionality, why ?13:53
dcalisteIndeed, I prefer the EAS way of doing things ;)13:53
flypigYes, I'm afraid I don't know the answer, and I agree with you about the EAS way being better.13:53
flypig(at least, on a cursory understanding).13:53
flypigOkay, it could be that when the notebook is set as isVisable == false, the calendar doesn't even appear as an option in the Calendar app.13:54
flypigI've just checked, and this is indeed the case.13:54
dcalisteAh, this is strange, because all notebooks should be listed at line 825 in calendarworker.cpp, or maybe not ?13:55
dcalisteWhat mkcal is listing as notebooks ? I'm checking.13:56
dcalisteAh, ok, I see, it's in nemo-qml-plugin-calendar/src/calendarworker.cpp:87714:00
dcalisteThe list of returned notebooks from mkcal are filtered out with tyhe isVisible() property.14:01
flypigThat makes sense.14:01
flypigAt least, is consistent with the results.14:01
dcalisteOk, so let this as it is and can I assume that you will modify contactsd to call notebook->setIsVisible(false) on account deactivation ?14:02
flypigCertainly, that's what I was planning. However, now you've flagged this up, if a calendar is disabled in the app, shouldn't it disable the alarms too?14:03
flypigI think if an account is disabled, it's reasonable for the Calendar not to appear as an option in the app, but if the account is enabled, but the calendar disabled, I think the alarms should also be disabled.14:04
flypigWhat do you think14:04
dcalisteI agree from the point of view of the user, but the code does not allow it easily at the moement, for the reason discussed above…14:05
flypigCould this turning on/off alarms be 'injected' imperatively into nemo-qml-plugin-calendar?14:06
flypigIn addition to it happening declaritively based on the notebook visibility. I guess that becomes problematic.14:07
dcalisteYes, I think so. I was a bit reluctant to do this, because it is spreading alarm gestion over two packages, but anyway, now that I think about it, {set,clear}Alarms() are already public in extendedStorage…14:07
flypigBut it would mean checking two settings, every time any of the settings changes.14:08
dcalisteAh true. So no imperative calls.14:09
dcalisteWhat about using this QSetting thing that is done in nemo-qml-plugin-calendar ? The excluded flag is saved in a QSetting there. One can read it from mkcal. It's ugly but can work as a preliminary step.14:10
flypigWell, I'm not sure. Maybe it needs some thought?14:10
dcalisteOtherwise, one can extend mkcal::Notebook API to add a "disable" property ?14:11
dcalisteOr, another possibility:14:12
flypigCurrently the status is stored in the notebook, using "nemo-qml-plugin-calendar" "exclude/...."14:13
dcalisteChange nemo-qml-plugin-calendar, to list all notebooks except those from disabled accounts and use the visibility as the current exclude property ?14:13
dcalisteBecause in loadNotebook(), we have access to accounts, since we're taking the account description for instance.14:14
dcalisteWhy not exclude notebook listing based on account enable property instead of notebook isVisible ?14:14
flypigLet me just process that. One second.14:15
flypigI wonder if this may have ramifications elsewhere. For example, do invisible notebooks for enabled accounts still sync?14:16
flypigIt may be a change that cascades.14:17
dcalisteThta's what I'm afraid of indeed. But, nothing should change from the user pov. Unchecked calendar of enabled account still sync (but don't set alarms), calendar of disabled account are not listed.14:19
dcalisteisVisible() is used instead of excluded in nemo-qml-plugin-calendar, alarms are hooked on isVisible() in mkcal, and calendar listing is modified in nemo-qmlplugin-calendar to exclude disabeld accounts instead of not isVisible() notebooks.14:20
flypigAnd mkcal amended to return all calendars independent of visibility.14:20
dcalistethat's already the case for mkcal. It's nemo-qml-plugin-calendar that exclude them.14:22
dcalisteAs a first step, I'm proposing to deal with a MR in mkcal to hook alarms and isVisible() property of a notebook.14:25
dcalisteAs a second step, it would be nice in account settings to use this isVisible() property to disable notebooks (as done already for EAS).14:26
flypigOne second, I'm just following that point about visibility being filtered by nemo-qml-plugin-calendar.14:26
dcalisteAs a possible third step, think about the possibility to use the isVisible() property instead of excluded in nemo-qml-plugin-calendar and filter out notebook listing on account enable prop instead of isVisible() notebook prop.14:27
dcalisteOups, sorry, yes, the notebooks are all listed by mkcal, irrelevant of visibility.14:28
dcalisteIt's in calendarworker.cpp:877 that the not visible notebooks are filtered out.14:28
flypigSorry, bear with me....14:32
flypigSomething is triggering the CalendarWorker::loadNotebooks(), but I can't see it right now.14:36
dcalisteIt's in the file itself, it's either the init() or when the storage is modified.14:38
dcalisteOn any write to the db, the notebooks are reloaded.14:38
flypigYes, I'm wondering which signal the storageModified attaches to.14:38
dcalistein nemo-qml-plugin-calendar.14:39
dcalisteAh, it's a slot in extendedstorageobserver from mkcal.14:39
dcalisteThe calendarworker inherit from this.14:40
flypigOkay, that's the key. Thank you.14:40
flypigSo, your first two proposed steps look uncontroversial to me. The third step I think just needs some care to avoid unwanted side effects. I think we'd have to check other places mkcal touches (homescreen, sync, calendar app, settings). If they all go through nemo-qml-plugin-calendar then it's fine, but if not they may need changes.14:44
dcalisteAs far as I know, they all use nemo-qml-plugin-calendar to ensure thread safety and load behind the scene. But I may be wrong, or missing something. pvuorela may know more about this.14:45
flypigFor example, we may have to check there isn't some peculiar circularity during account setup (calendar exists, account not yet enabled).14:45
flypigThat's just speculation, but the sort of thing I can imagine might be unexpected.14:46
dcalisteSure, I'm sharing your concern. I don't like modifying behaviour like that. That's why I wanted to discuss the matter with you before changing anything.14:47
flypigI propose we go with the first two steps (I don't see any other sensible approach), and just leave the third step to settle. It looks like a good idea, but I'd feel more comfortable after having pondered and discussed it again.14:50
flypigWould that be okay?14:50
dcalisteSure, thanks for shqring your thoughts with me.14:51
flypigHmmm. Actually, I'm oversimplifying. One second...14:52
flypigStep two and step three have to be the same step I think.14:52
flypigOtherwise, disabling a calendar will make it disappear from the settings app.14:53
dcalisteThe step two I was thinking about is the call to setIsVisible() for other accounts than EAS (like caldav…).14:53
dcalisteThis makes disabled account not visible and not ringing anymore.14:54
dcalisteStep three is to remove ringing for unchecked calendars in calendar app.14:54
dcalisteI mean, by modifying nemo-qml-plugin-calendar to also use isVisible() for excluded notebooks.14:54
flypigSorry, I need to break that down a bit more.14:56
flypig"The step two I was thinking about is the call to setIsVisible() for other accounts than EAS (like caldav…) when enabling/disabling an account"14:56
flypig"The step two I was thinking about is the call to setIsVisible() for other accounts than EAS (like caldav…) when selecting/deselecting a notebook under Calenders on the Settings page for CalDAV"14:57
dcalisteFirst proposition : when account is enabled / disabled.14:57
dcalisteSecond proposition is for step three, because it may have unexpected consequences if implemented by changing the isVisible / excluded in calendarworker.14:58
flypigSo, I'm now just thinking about the transition from step 2 to step 3 again.15:00
flypigSorry to go round in circles on this. I think it might be a converging spiral ;)15:03
flypigIf isVisible is used to filter out calendars shown in nqpc, what will be used to disable alarms when an account is disabled?15:03
dcalisteThat's why step 2 is important. Account settings will have to both disable account (so they are not listed) _and_ setIsVisible(false), so alarms are removed.15:05
dcalisteTwo actions must disable alarms : account disabling or calendar unchecked in app.15:06
dcalisteBut only one action can be hooked to remove alarms.15:06
dcalisteSo the hookig action is a call to setIsVisible(false), and thus both account settings and nemo-qml-plugin-calendar have to call this.15:07
dcalisteWell, that's my proposition ;)15:07
flypigYes, okay, I'm back on board again.15:11
flypigThat's clear.15:11
flypigAre you happy to continue with step 1? I already started on step 2.15:12
dcalisteYeh, I'm going to submit a MR in mkcal today or tomorrow, but most probably tomorrow, if I want to add tests.15:13
flypigOkay, great. Let's return to step 3 at that point then. I think you persuaded me, but we should review it.15:14
dcalisteSure, I agree.15:14
flypigThanks for taking the time, as always, to explain things!15:14
dcalisteThanks for the exchange, I was a bit undecided.15:15
oozboozHi.. my XA2 connects to car BT fine but car panel does not show any metadata (song, artist, etc) and car media controls do not work either (stop/start music)15:27
attahcoderus: I'm about to write a TJC post complaining about "that apps invoked by their appname:// uri doesn't get passed the pkg parameter", is this a fair and understandable description?18:19
attah(i can't see that aliendalvikcontrol has broken anything, but i also haven't compared back and forth, so i'm blaming the runtime)18:28

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