| *** zbenjamin_ is now known as zbenjamin | 02:26 | |
| dcaliste | Hello chriadam, I hope you spent pleasant holidays. | 08:06 |
|---|---|---|
| chriadam | hi dcaliste, yes it was a nice vacation, thanks! | 08:07 |
| chriadam | I hope your February was pleasant also | 08:07 |
| chriadam | I 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 anyway | 08:09 |
| dcaliste | Yeh, it was alright. If we look at what was left before your vacations, not much in fact : mainly kcalcore!15 | 08:09 |
| chriadam | I will check that | 08:09 |
| dcaliste | About 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 |
| dcaliste | There are few additions in nemo-qml-plugin-calendar to export the time spec and time zone. | 08:10 |
| dcaliste | And following pvuorela advice, I added to entries in the time zone picker (none and UTC) and added a value button in EventEditPage.qml | 08:11 |
| chriadam | excellent, sounds like that one is progressing very nicely, thanks for that | 08:11 |
| dcaliste | The modifications in nemo-qml-plugin-calendar are worth discussing though | 08:11 |
| dcaliste | In fact there is currently an inconsistency in the event edit page: | 08:11 |
| chriadam | kcalcore!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 |
| dcaliste | If 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 |
| dcaliste | if 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 |
| dcaliste | This 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 |
| dcaliste | This is inconsistent. | 08:17 |
| chriadam | for 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 |
| dcaliste | I'm speaking at the moment without my modifications. | 08:17 |
| chriadam | right | 08:17 |
| dcaliste | These are the current state of calendar UI. | 08:17 |
| dcaliste | My point is : | 08:18 |
| dcaliste | - it should good to use the toLocalZone() conversion for the plain events also, but | 08: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 |
| dcaliste | So 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 |
| dcaliste | The second possibilities, requires that UTC and occurrences can return the time in the parent time zone. | 08:21 |
| dcaliste | If 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_67 | 08:21 |
| chriadam | when 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 |
| dcaliste | for the UTC part. | 08:21 |
| dcaliste | You're right, that's my point. | 08:22 |
| dcaliste | It's conflicting. | 08:22 |
| dcaliste | There are two possibilities to solve the inconsistency, but they are mutually exclusive. | 08:22 |
| dcaliste | I'm in favour of my patch and solution of course, but it's fair to expose the other one also. | 08:23 |
| chriadam | ok, 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 |
| chriadam | does it ensure that the timezone info is exposed in the event and occurrence data to QML? | 08:23 |
| chriadam | and then always show the start date etc in the appropriate timespec for that timezone? | 08:24 |
| dcaliste | Yes, for UTC fix, follow the link above, and for the occurrences, it is... | 08:24 |
| dcaliste | https://git.sailfishos.org/mer-core/nemo-qml-plugin-calendar/merge_requests/51/diffs#7752bbaf0c28e762038a50c57f622b5eb8d26681_73_97 | 08:24 |
| dcaliste | For 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 |
| dcaliste | For occurrence, one need to move the QDateTime of the occurrence to the parent time zone. | 08:26 |
| dcaliste | And export it to QML via another property than startTime (which is in local zone). | 08:26 |
| *** frinring_ is now known as frinring | 08:27 | |
| dcaliste | The name I choose for the new property is _really_ bad, but I had no better idea. | 08:27 |
| dcaliste | occurrence 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 |
| chriadam | what 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 |
| dcaliste | occurrences in nemo-qml-plugin-calendar, as far as I undestand, has a slightly different meaning than occurrences in mkcal. | 08:28 |
| chriadam | pvuorela: 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 localtz | 08:29 |
| dcaliste | They 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 |
| dcaliste | For recurring ones, they are plenty, including exceptions. | 08:30 |
| chriadam | I see | 08:30 |
| dcaliste | I though about changing the meaning of occurrence.startTime to be consistent, but I'm afraid of unexpected consequences. | 08:31 |
| dcaliste | To 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 |
| dcaliste | Which is fine. | 08:32 |
| chriadam | indeed. 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 |
| dcaliste | Only the editing page is using the event.startTime for plain events and occurrence.startTime for specific occurrence of a recurring series. | 08:33 |
| dcaliste | To 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 |
| chriadam | I 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 |
| dcaliste | For 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 |
| chriadam | aaah I see | 08:38 |
| dcaliste | That's also why, having event.startTime and eventoccurrence.startTime being not in the same time reference makes sense also. | 08:39 |
| dcaliste | A 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 |
| chriadam | sure. 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 |
| dcaliste | Yes, 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 |
| chriadam | hrm, good question :-( | 08:43 |
| chriadam | I think there is a test flag, it creates a db in the cwd | 08:44 |
| chriadam | but not sure whether the nqpc tests make use of that etc | 08:44 |
| dcaliste | Normally 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 |
| dcaliste | So I guess I can do the same here. But didn't look deep enough yet to convinced myself. | 08:45 |
| chriadam | yep, 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 |
| dcaliste | Thanks. 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 |
| chriadam | I 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 yet | 08:49 |
| dcaliste | Before concluding the meeting, there are two things that I would like to quickly discuss : | 08:49 |
| chriadam | definitely | 08:49 |
| dcaliste | - https://git.sailfishos.org/mer-core/buteo-syncfw/merge_requests/45 | 08: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/36 | 08:50 |
| dcaliste | (adding a way to export comm history for unregistered number) | 08:50 |
| chriadam | blam and Venemo know more about commhistory than I do at this point | 08:51 |
| Venemo | I usually just hit it with a hammer until it starts doing what I want | 08:52 |
| chriadam | ;-) | 08:52 |
| dcaliste | Venemo, yeh, that's the problem. The point here is how to allow to reuse the code from the singlecontactevent for unsaved numbers. | 08:53 |
| chriadam | dcaliste: 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 forward | 08:53 |
| dcaliste | pvuorela notices that using inheritance as I'm proposing is not very elegant. Which I agree, it has some drawbacks. | 08:53 |
| Venemo | I can't give you a good answer dcaliste without reading the code myself | 08:54 |
| dcaliste | So guys, if you have an idea (after reading the code of course), please comment in the MR. | 08:54 |
| Venemo | but I can do this if you feel that it's important | 08:54 |
| dcaliste | I put the link again here : https://git.sailfishos.org/mer-core/libcommhistory/merge_requests/36 | 08:55 |
| Venemo | yeah, already clicked | 08:55 |
| chriadam | dcaliste: 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 |
| chriadam | thanks Venemo | 08:55 |
| Venemo | though I don't exactly see what this is set to accomplish, I can already see the unsaved numbers in my phone app | 08:56 |
| dcaliste | I 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 |
| dcaliste | Tap on an unsaved number to open the card. | 08:57 |
| dcaliste | At the moment, you only have the number, but no history, like for a contact. | 08:57 |
| Venemo | if I tap it it just calls it again | 08:58 |
| dcaliste | With the patch, you can also see comm history for unsaved numbers, and see when you where called and hoa many times. | 08:58 |
| Venemo | ah, I see. | 08:58 |
| dcaliste | Ah, yeh, I use the option "tap to view the contact card, don't call back". | 08:58 |
| Venemo | you mean you have "quick call" turned off | 08:58 |
| dcaliste | Right. | 08:58 |
| Venemo | I think then the call ui would need to be patched to add "show contact card" on unsaved numbers, too, to bring it up to parity | 08:59 |
| Venemo | but yeah, I think I get it now | 08:59 |
| dcaliste | Good point, I can see to add this indeed. Thanks for pointing it to me. | 08:59 |
| Venemo | though it's also a good question if the term "contact card" is something that makes sense for an unsaved contact | 09:00 |
| dcaliste | Sure, can think of a better wording for unsaved numbers. | 09:00 |
| Venemo | dunno. it is a contact card, after all. | 09:01 |
| Venemo | chriadam: any idea? | 09:01 |
| chriadam | I think contact card is fine nomenclature, personally, but haven't given it much thought | 09:01 |
| Venemo | okay | 09:01 |
| chriadam | I agree that some "show contact card" action is needed for such unsaved contacts | 09:01 |
| chriadam | the main concern I have is about the implementation, as per Pekka's comments | 09:02 |
| chriadam | but that can be iterated / discussed | 09:02 |
| Venemo | yeah, sure | 09:04 |
| chriadam | dcaliste: thanks very much for working on that! much appreciated. | 09:04 |
| chriadam | was there anything else to discuss? | 09:04 |
| flypig | Can I also please ask a question of you guys about calendars? | 09:04 |
| dcaliste | Ok, that's all for today. Thanks everyone for the feedback and discussions. | 09:04 |
| Venemo | also, 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 honest | 09:04 |
| chriadam | flypig: go for it | 09:04 |
| dcaliste | flypig: sure | 09:05 |
| flypig | Thanks :) I'm looking at the issue of calendars remaining in the calendar app even when they're disabled. | 09:05 |
| dcaliste | flypig: do you mean the account is disabled ? | 09:05 |
| flypig | Yes, 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 |
| chriadam | but maybe it's time to revisit that decision | 09:06 |
| dcaliste | Venemo, thanks, but no problem discussing contributions, it's always fruitful and I often learn things. | 09:06 |
| flypig | It I'm not sure whether it's intentional, but it's at least inconsistent, because it does it for EAS. | 09:06 |
| Venemo | dcaliste: yeah, it's just the feeling I get when I look at libcommhistory :P | 09:07 |
| flypig | It doesn't seem to delete the data, just doesn't show it any more. | 09:07 |
| chriadam | flypig: that would be the ideal solution, IMO. hide without deleting. | 09:07 |
| flypig | And that's different from disablying sync, which leaves the entries there (just to clarify the terminology). | 09:07 |
| flypig | chriadam, yes, I agree. | 09:07 |
| flypig | It doesn't look so hard to achieve, because notebooks can be disabled in mkcal. This is how EAS does it, I think. | 09:08 |
| dcaliste | flypig, I think the pipe work is in buteo msync/AccountsHelper.cpp | 09:08 |
| dcaliste | Let me give a look... | 09:08 |
| chriadam | flypig: 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 |
| flypig | Could you elaborate please chriadam? | 09:09 |
| chriadam | flypig: if you want to fix it for calendars, I hope you also fix it for contacts, images, eventsview entries, etc ;-P | 09:09 |
| flypig | dcaliste, 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 |
| dcaliste | flypig, ah no, it's not in AccountsHelper.cpp, it is only responsible for data deletion on account deletetion... | 09:10 |
| chriadam | flypig: 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 appropriately | 09:10 |
| chriadam | similarly, for contacts, we would need some "accountHidden" flag associated with each contact or whatever, and not return it in filter requests etc | 09:10 |
| chriadam | for consistency | 09:10 |
| chriadam | with hidden calendars | 09:11 |
| flypig | Yes, 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 |
| chriadam | hah. | 09:12 |
| chriadam | problematic. | 09:12 |
| flypig | As it happens, hiding calendar entries won't fix the problem, but the two do seem to conceptually go together. | 09:12 |
| dcaliste | flypig, I think it's mkcal which should disable alarms on disabled notebooks. | 09:13 |
| chriadam | I 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 |
| flypig | chriadam, yes, exactly. | 09:13 |
| chriadam | dcaliste: right, mkcal has some link to timed iirc | 09:13 |
| flypig | dcaliste, yes, that makes sense, but I don't think it happens right now. | 09:13 |
| dcaliste | But I'm pretty sure mkcal is not doing it at the moment, just doing it when deleting entries. | 09:14 |
| flypig | dcaliste, correct: EAS disbles mkcal and hides events, but alarms continue to go. | 09:14 |
| flypig | whereas other accounts do neither. | 09:14 |
| flypig | i.e. other accounts continue to show and alarms continue to trigger. | 09:15 |
| dcaliste | dding the code in mkcal to stop alarms for disabled events and disbaled notebooks is then a first step. | 09:15 |
| chriadam | flypig: mkcal ExtendedStorage::clearAlarms(const QString ¬ebookUid) | 09:16 |
| flypig | dcaliste, yes, although I'm approaching it from the other side, trying to figure out where to disable mkcal events. | 09:16 |
| flypig | chriadam, dcaliste, great, that looks good :) | 09:16 |
| dcaliste | What do you mean disable mkcal events ? | 09:16 |
| chriadam | he means "mkcal-sourced timed events" I think | 09:16 |
| flypig | dcaliste: 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 |
| chriadam | flypig: ah. | 09:17 |
| dcaliste | flypig, yes, that's my second step ;) | 09:17 |
| chriadam | previously, we stuck that kind of glue into contactsd | 09:17 |
| chriadam | just because it exists | 09:17 |
| *** Renault_ is now known as Renault | 09:18 | |
| chriadam | in the mystical future when we have daemonised system APIs (e.g. offer system apis via IPC), the calendar service would do that | 09:18 |
| flypig | chriadam, 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 |
| dcaliste | But I have no idea just now where to put this second step of mine. | 09:18 |
| chriadam | I would put it into contactsd, personally | 09:18 |
| flypig | The options seem to be: contactsd, buteo, something new. | 09:19 |
| chriadam | not buteo | 09:20 |
| flypig | inappropriate? | 09:20 |
| chriadam | I think so. it's not a sync plugin, and buteo plugins should be sync plugins IMO | 09:20 |
| dcaliste | Buteo 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 |
| flypig | Okay, 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 |
| dcaliste | And 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 |
| chriadam | as 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 |
| chriadam | there will come a time when I will take a large axe to contactsd and remove all of the telepathy/jabber stuff | 09:21 |
| chriadam | but its other functions (watching for changes and triggering appropriate sync profiles; birthdays plugin) remain viable. | 09:22 |
| flypig | Okay, so I'll look at contactsd and try to fit it in there then? | 09:22 |
| chriadam | that'd be my first suggestion, yes. | 09:22 |
| flypig | Okay, great, thanks, that's very helpful. | 09:22 |
| chriadam | but feel free to ignore my suggestion | 09:22 |
| dcaliste | I'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 |
| flypig | Well, 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 |
| chriadam | dcaliste: 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 |
| dcaliste | There is can call mkcal directly like I did in jolla-calendar to get the account id from notebooks. | 09:23 |
| chriadam | the 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 think | 09:24 |
| dcaliste | s/is/it/ | 09:24 |
| flypig | Sorry, I lost the line of thought here. | 09:25 |
| flypig | Are there are two new options here? | 09:25 |
| chriadam | flypig: 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 UI | 09:25 |
| flypig | Are yes, I see. | 09:25 |
| flypig | I looked at that, but it seems problematic with the current implementation. | 09:26 |
| chriadam | my concern with that is: MDM | 09:26 |
| flypig | Yes, that's also a good point. | 09:26 |
| flypig | Ideally, it would happen wherever the account is disabled from. | 09:26 |
| dcaliste | Which may no be a good idea neither since accounts may be disabled outside the UI... | 09:26 |
| flypig | But when an account is disabled, there's generic code, and no hook to add anything specific. | 09:26 |
| chriadam | right. I think contactsd listener makes sense. give it a crack and see how far you get ;-) | 09:26 |
| dcaliste | I don't know contactsd, so if chriadam thinks it's a good place, that's fine with me. | 09:27 |
| chriadam | it's... awful of course. but it exists ;-) | 09:27 |
| dcaliste | If you need help for the mkcal part to remove alarms of disabled events, tell me. | 09:27 |
| flypig | Great, 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 |
| flypig | Oh, sorry, crossed messages. Thank you dcaliste! | 09:27 |
| chriadam | should hopefully just be a call to that mkcal ExtendedStorage method | 09:28 |
| dcaliste | chriadam, I agree. | 09:28 |
| chriadam | after looking up the notebooks associated with the given account id | 09:28 |
| flypig | Okay, that sounds relatively straightforward. If I hit issues, I'll get back to you for sure. | 09:28 |
| chriadam | sounds good :-) | 09:29 |
| chriadam | ok, anythign else to discuss? if not, thanks all, and have a great week! | 09:29 |
| dcaliste | chriadam, have a good evening, see you later. | 09:29 |
| flypig | Thank you again! | 09:29 |
| dcaliste | flypig: 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 |
| flypig | dcaliste, 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 |
| dcaliste | Ok, I will try to create a MR for that before the end of the week. | 09:35 |
| flypig | dcaliste, 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 |
| dcaliste | flypig, do you have some time to discuss the notebook visible issue in mkcal and friends ? | 13:43 |
| flypig | dcaliste, sure. | 13:44 |
| flypig | What were your thoughts? | 13:44 |
| dcaliste | I'm looking at changing the alarm hooking in mkcal depending on notebook visibility. | 13:44 |
| flypig | Okay, great, sounds good. | 13:44 |
| dcaliste | Which consequently, bring me to how UI is handling that in calendar setting page ? | 13:44 |
| flypig | How UI changes enabled/disabled status? | 13:45 |
| dcaliste | It is using a excludedNotebook string array that is passed to nemo-qml-plugin-calendar. | 13:45 |
| flypig | One second: which account type are you looking at, and which option? | 13:45 |
| dcaliste | Then, inside the calendarworker, ::eventOccurrences() is filtering out events returned by mkcal depending on this excluded list. | 13:46 |
| dcaliste | I'm looking at code in ui-jolla-calendar/application/pages/SettingPages.qml | 13:46 |
| dcaliste | The calendar setting page in the calendar app, to switch notebook visibility. | 13:46 |
| dcaliste | Toggling the switch there then is changing the excludedNotebook property of the CalendarManager from nemo-qml-plugin-calendar. | 13:47 |
| flypig | Ah yes, okay, now I understand... I'm wondering if that's how it's done with EAS though. that may be different. | 13:47 |
| flypig | But anyway, I'm with you now. | 13:47 |
| dcaliste | Ok, then… | 13:48 |
| dcaliste | Incalendarworker.cpp::eventOccurrences() the event list returned by mkcal is filtered out to remove occurrences from excluded notebooks. | 13:49 |
| flypig | Yes, I see the code. | 13:50 |
| dcaliste | My 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 |
| dcaliste | Because 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 |
| flypig | EAS seems to do something more in line with what you're suggesting: | 13:52 |
| flypig | notebook->setIsVisible(visibleState); | 13:52 |
| dcaliste | Is 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 |
| dcaliste | Indeed, I prefer the EAS way of doing things ;) | 13:53 |
| flypig | Yes, 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 |
| flypig | Okay, 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 |
| flypig | I've just checked, and this is indeed the case. | 13:54 |
| dcaliste | Ah, this is strange, because all notebooks should be listed at line 825 in calendarworker.cpp, or maybe not ? | 13:55 |
| dcaliste | What mkcal is listing as notebooks ? I'm checking. | 13:56 |
| dcaliste | Ah, ok, I see, it's in nemo-qml-plugin-calendar/src/calendarworker.cpp:877 | 14:00 |
| dcaliste | The list of returned notebooks from mkcal are filtered out with tyhe isVisible() property. | 14:01 |
| flypig | That makes sense. | 14:01 |
| flypig | At least, is consistent with the results. | 14:01 |
| dcaliste | Ok, 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 |
| flypig | Certainly, 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 |
| flypig | I 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 |
| flypig | What do you think | 14:04 |
| flypig | ? | 14:04 |
| dcaliste | I 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 |
| flypig | Could this turning on/off alarms be 'injected' imperatively into nemo-qml-plugin-calendar? | 14:06 |
| flypig | In addition to it happening declaritively based on the notebook visibility. I guess that becomes problematic. | 14:07 |
| dcaliste | Yes, 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 |
| flypig | But it would mean checking two settings, every time any of the settings changes. | 14:08 |
| dcaliste | Ah true. So no imperative calls. | 14:09 |
| dcaliste | What 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 |
| flypig | Well, I'm not sure. Maybe it needs some thought? | 14:10 |
| dcaliste | Otherwise, one can extend mkcal::Notebook API to add a "disable" property ? | 14:11 |
| dcaliste | Or, another possibility: | 14:12 |
| flypig | Currently the status is stored in the notebook, using "nemo-qml-plugin-calendar" "exclude/...." | 14:13 |
| dcaliste | Change nemo-qml-plugin-calendar, to list all notebooks except those from disabled accounts and use the visibility as the current exclude property ? | 14:13 |
| dcaliste | Because in loadNotebook(), we have access to accounts, since we're taking the account description for instance. | 14:14 |
| dcaliste | Why not exclude notebook listing based on account enable property instead of notebook isVisible ? | 14:14 |
| flypig | Let me just process that. One second. | 14:15 |
| flypig | I wonder if this may have ramifications elsewhere. For example, do invisible notebooks for enabled accounts still sync? | 14:16 |
| flypig | It may be a change that cascades. | 14:17 |
| dcaliste | Thta'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 |
| dcaliste | isVisible() 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 |
| flypig | And mkcal amended to return all calendars independent of visibility. | 14:20 |
| dcaliste | that's already the case for mkcal. It's nemo-qml-plugin-calendar that exclude them. | 14:22 |
| dcaliste | As a first step, I'm proposing to deal with a MR in mkcal to hook alarms and isVisible() property of a notebook. | 14:25 |
| dcaliste | As 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 |
| flypig | One second, I'm just following that point about visibility being filtered by nemo-qml-plugin-calendar. | 14:26 |
| dcaliste | As 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 |
| dcaliste | Oups, sorry, yes, the notebooks are all listed by mkcal, irrelevant of visibility. | 14:28 |
| dcaliste | It's in calendarworker.cpp:877 that the not visible notebooks are filtered out. | 14:28 |
| flypig | Sorry, bear with me.... | 14:32 |
| flypig | Something is triggering the CalendarWorker::loadNotebooks(), but I can't see it right now. | 14:36 |
| dcaliste | It's in the file itself, it's either the init() or when the storage is modified. | 14:38 |
| dcaliste | On any write to the db, the notebooks are reloaded. | 14:38 |
| flypig | Yes, I'm wondering which signal the storageModified attaches to. | 14:38 |
| dcaliste | in nemo-qml-plugin-calendar. | 14:39 |
| dcaliste | Ah, it's a slot in extendedstorageobserver from mkcal. | 14:39 |
| dcaliste | The calendarworker inherit from this. | 14:40 |
| flypig | Okay, that's the key. Thank you. | 14:40 |
| flypig | So, 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 |
| dcaliste | As 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 |
| flypig | For example, we may have to check there isn't some peculiar circularity during account setup (calendar exists, account not yet enabled). | 14:45 |
| flypig | That's just speculation, but the sort of thing I can imagine might be unexpected. | 14:46 |
| dcaliste | Sure, 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 |
| flypig | I 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 |
| flypig | Would that be okay? | 14:50 |
| dcaliste | Sure, thanks for shqring your thoughts with me. | 14:51 |
| flypig | Hmmm. Actually, I'm oversimplifying. One second... | 14:52 |
| flypig | Step two and step three have to be the same step I think. | 14:52 |
| flypig | Otherwise, disabling a calendar will make it disappear from the settings app. | 14:53 |
| dcaliste | The step two I was thinking about is the call to setIsVisible() for other accounts than EAS (like caldav…). | 14:53 |
| dcaliste | This makes disabled account not visible and not ringing anymore. | 14:54 |
| dcaliste | Step three is to remove ringing for unchecked calendars in calendar app. | 14:54 |
| dcaliste | I mean, by modifying nemo-qml-plugin-calendar to also use isVisible() for excluded notebooks. | 14:54 |
| flypig | Sorry, 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 | or | 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 |
| flypig | ? | 14:57 |
| dcaliste | First proposition : when account is enabled / disabled. | 14:57 |
| dcaliste | Second proposition is for step three, because it may have unexpected consequences if implemented by changing the isVisible / excluded in calendarworker. | 14:58 |
| flypig | So, I'm now just thinking about the transition from step 2 to step 3 again. | 15:00 |
| flypig | Sorry to go round in circles on this. I think it might be a converging spiral ;) | 15:03 |
| flypig | If isVisible is used to filter out calendars shown in nqpc, what will be used to disable alarms when an account is disabled? | 15:03 |
| dcaliste | That'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 |
| dcaliste | Two actions must disable alarms : account disabling or calendar unchecked in app. | 15:06 |
| dcaliste | But only one action can be hooked to remove alarms. | 15:06 |
| dcaliste | So 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 |
| dcaliste | Well, that's my proposition ;) | 15:07 |
| flypig | Yes, okay, I'm back on board again. | 15:11 |
| flypig | That's clear. | 15:11 |
| flypig | Are you happy to continue with step 1? I already started on step 2. | 15:12 |
| dcaliste | Yeh, I'm going to submit a MR in mkcal today or tomorrow, but most probably tomorrow, if I want to add tests. | 15:13 |
| flypig | Okay, great. Let's return to step 3 at that point then. I think you persuaded me, but we should review it. | 15:14 |
| dcaliste | Sure, I agree. | 15:14 |
| flypig | Thanks for taking the time, as always, to explain things! | 15:14 |
| dcaliste | Thanks for the exchange, I was a bit undecided. | 15:15 |
| oozbooz | Hi.. 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 |
| attah | coderus: 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/!