| *** zbenjamin is now known as Guest23312 | 01:19 | |
| *** zbenjamin_ is now known as zbenjamin | 01:19 | |
| dcaliste | Hello chriadam, I woke up earlier today. If you're free, we may discuss from now and save you time in the evening ? | 06:03 |
|---|---|---|
| chriadam | hi dcaliste, sure! | 06:03 |
| chriadam | I hope you had a good week! | 06:03 |
| dcaliste | It was fine, thank you. I wish it was as good for you also. | 06:04 |
| chriadam | yes, wasn't too bad | 06:05 |
| dcaliste | Compared to last week, there are still the two MR changing the listing of notebooks in the calendar app, based on account services instead of notebook visibility. | 06:06 |
| dcaliste | Now that I write it, I think that I still didn't push a commit on top to silence the warning ahen an event has been removed from the manager and the agenda model tries to access it. | 06:07 |
| dcaliste | As we discussed the meeting before. | 06:07 |
| chriadam | so those two are nqpc#55 and bsps#58 | 06:09 |
| dcaliste | Exactly, that's them. | 06:09 |
| chriadam | great - I think they both look good, have to give them another test before merging. but I guess if you're intending to make a change there, I'll hold off on that for now | 06:10 |
| dcaliste | Yes, I'll try to push an additional commit to silence the warning when inquiring for an event that has been removed already. Will do it today. | 06:11 |
| chriadam | no rush, thank you | 06:11 |
| chriadam | I see that nqpc#51 is mostly ok, but was waiting on comment from pvuorela regarding property naming? | 06:11 |
| chriadam | that's separate of course | 06:11 |
| dcaliste | Yes, that's the current status. The naming is really bad at the moment but I cannot find anything better. | 06:12 |
| chriadam | I will poke Pekka again about that one ;-) | 06:13 |
| chriadam | aside from that, I saw the "monthly recurrence on week-day" PRs (jolla-calendar#257 and nqpc#56) | 06:13 |
| chriadam | I haven't had a chance to review them though | 06:13 |
| dcaliste | It's in CalendarEventOccurrence, we need to differentiate the date/time given in the local time zone (normal start and end properties) from the date / time in the event time zone, because I'm not sure JS can handle the conversion itself. | 06:14 |
| dcaliste | Yes, for the last two MRs, I'm trying to implement some mockups provided by Bongo on TJC. | 06:14 |
| dcaliste | The MR is introducing the recurrence on day of week in a monthly recurrence. | 06:15 |
| dcaliste | Something like "second Tuesday every month". | 06:15 |
| chriadam | makes sense, something that has been missing for a while, thanks | 06:16 |
| dcaliste | The support already exists in KCalCore, I've just added the required bindings in the middleware. | 06:16 |
| dcaliste | It's done as a new value in the Recur enum. | 06:16 |
| chriadam | great | 06:16 |
| dcaliste | Then, the position and the day are taken from the dtstart value. | 06:17 |
| chriadam | Pekka also mentioned that Bea and I should try to progress your "list activity for unsaved contacts" PR - need to discuss with her about that one, as I'm not too familiar with libcommhistory | 06:17 |
| dcaliste | KCalCore allows more flexibility, but it's useless in our case, I guess. We put the constraint on the week day of dtstart and the position in the month from the same dtstart value. | 06:18 |
| dcaliste | There is only one difference with the mockups in TJC, is that we cannot decide between the fourth week or the last. It's automatically chosen to be the last. | 06:19 |
| dcaliste | The mockups propose to add two possibilities in that case, the fourth and the last, but it complicates the implementation by adding a second enum value to differentiate both cases. | 06:20 |
| dcaliste | So I decided to go first to an automatic switch to "last" and not giving the possibility to choose between fourth and last. | 06:20 |
| chriadam | I agree, let's keep it simple initially. it's already a big improvement feature. | 06:21 |
| dcaliste | Yeh, and code addition is minor. | 06:21 |
| dcaliste | About the other recurring feature that I would like to add from the mockups is the "selected days". | 06:21 |
| dcaliste | Meaning that you recurs weekly on some selected days, like work days, or Monday, Tuesday, Thursday and Friday, because you have kids on Wednesday. | 06:22 |
| dcaliste | Things like that. | 06:22 |
| chriadam | right, training every tuesday and thursday etc | 06:22 |
| dcaliste | KCalCore proposes the same API as the monthlyByDayOfWeek approch. | 06:23 |
| dcaliste | So it's easy to follow the same path, but... | 06:23 |
| dcaliste | choosing the days, looks like a bit array. So there are two possibilities: | 06:23 |
| dcaliste | - add a new property in CalendarEvent, storing this bit array, | 06:23 |
| chriadam | by bit array, is it just days in week (e.g. 7 bits) or is it days in month (e.g. 31 bits)? | 06:24 |
| dcaliste | - abuse the Recur Enum by booking 127 values and doing the bit array from there. | 06:24 |
| dcaliste | It's days in week. | 06:24 |
| dcaliste | so 127 possibilities. | 06:24 |
| chriadam | so effectively, only requires adding an extra unsigned char to the CalendarEvent struct | 06:25 |
| dcaliste | Myself, I would prefer the second solution, but I would like to ask your opinion. | 06:25 |
| dcaliste | You prefer yourself to add only one value in the Recur enum, and inquire another property for the days ? | 06:26 |
| chriadam | I haven't thought it through properly yet, but I worry that (since the Recur enum presumably already has some values) we'd have to extend the enum, then perform bit-shift manipulation to get from the zero-based bit array values to the extended enum value | 06:26 |
| chriadam | but perhaps I'm misunderstanding | 06:27 |
| dcaliste | No that's the idea ;) That's why I phrase "abuse". There is no bit shift to do though, just substract the enum value of the RecurWeeklyByDays, and then do bit mask on the result. | 06:28 |
| dcaliste | But having a new property is fine with me also, and less convoluted. | 06:28 |
| chriadam | at first glance, I think I would prefer a new property. I guess the problem with that is that the client needs to be aware that querying its value only makes sense if the other property's value is the "selectedDays" enum value.. | 06:29 |
| chriadam | probably the lesser of two evils. can return 0 in that case anyway | 06:30 |
| dcaliste | Oh, that's allright I think, because the values will be used only for a specific entry in the combobox values, as in the mockups. Indeed, the return 0 value will do the job when not used. | 06:30 |
| dcaliste | Ok, so I'll go for the new property then. Thanks for the direction. | 06:31 |
| dcaliste | About the libcommhistory MR, don't hesitate to ask anything, or Bea too also. | 06:32 |
| chriadam | thank you very much, I expect we'll get to it later this week | 06:33 |
| dcaliste | The main issue, why it was not working before, is an API issue. | 06:33 |
| dcaliste | The db inquire is done on phone numbers anyway. | 06:33 |
| dcaliste | But the API just allow to fetch history from a contact id. | 06:34 |
| dcaliste | My proposition was to add an API to pass a phone number also. | 06:34 |
| chriadam | perhaps related, I know that Venemo has done extensive work on phone number matching, both in libcommhistory but also e.g. voicecall / contacts / etc, so perhaps there is some capability to add API for "match by phone number" now better | 06:34 |
| chriadam | I agree that adding some information (e.g. phone number) makes sense if there is no contact id associated. in general, I think it's useful to know which channel was used anyway (one person could have multiple devices etc) | 06:35 |
| chriadam | so, sounds good to me ;-) but let's see what is raised during the discussion | 06:35 |
| dcaliste | The only thing "strange" in the MR that makes it long to be accepted (with or without modifications), is that the name of the class implies a contact. | 06:36 |
| dcaliste | So it's strange to add a function to fetch on a phone number. | 06:36 |
| dcaliste | My proposition was then to add an intermediate new class that works on phone numbers. | 06:37 |
| dcaliste | The already existing contact history listing is then just a specialisation of the phone number history listing. | 06:37 |
| chriadam | sounds reasonable also | 06:37 |
| dcaliste | Yeh, but my C++ skills are not as good as it could be and I mess up a bit with virtual classes. That's what pvuorela raised. | 06:38 |
| dcaliste | s/virtual classes/virtual methods/ | 06:39 |
| dcaliste | And i'm a bit stuck there. | 06:39 |
| Venemo | good morning. | 06:39 |
| chriadam | ah, all good, we'll take a look | 06:39 |
| chriadam | hi Venemo | 06:39 |
| dcaliste | Hello Venemo. | 06:39 |
| Venemo | sorry I missed this meeting, just saw you just highlighted me :P | 06:40 |
| chriadam | we started early today | 06:40 |
| dcaliste | Thanks chriadam, and once again, don't hesitate to ask, on IRC, any question if you find something strange or not understandable in the MR. | 06:40 |
| chriadam | just briefly discussing dcaliste's MR in libcommhistory | 06:40 |
| Venemo | what exactly are you proposing wrt phone number matching, dcaliste ? | 06:40 |
| chriadam | allowing user to see communication events with a phone number which hasn't been saved as a contact | 06:41 |
| dcaliste | It's not exactly about phone number matching, it's about allowing to fetch history for a given number. | 06:41 |
| Venemo | I see | 06:41 |
| dcaliste | Currently, contact history listing is done by fetching all comm events for all phone numbers of the contact. | 06:41 |
| dcaliste | I'm proposing to add an API to directly attack the db query from a single phone number. | 06:42 |
| Venemo | well, I didn't change the API around it, only the implementation. so the number matching will be more accurate under the hood, but the mechanism to use it hasn't changed | 06:42 |
| dcaliste | So we can list the history of an unsaved contact without changing anything in the above layers. | 06:42 |
| chriadam | :-) | 06:43 |
| chriadam | did you have anything else to discuss this week? | 06:43 |
| Venemo | for example, previously we (mistakenly) matched any two numbers that differed only in the area code, and now that won't happen anymore | 06:43 |
| dcaliste | Yeh, I think the two works are orthogonal. Yours making all of this much more robust. | 06:43 |
| Venemo | that's my thought as well | 06:44 |
| dcaliste | chriadam, I think that's all for me that week. I'll add the warning mitigation commit today, and then work during the week on the selected day recurrence. | 06:44 |
| chriadam | thanks very much, and thank you as always for your time and effort | 06:45 |
| chriadam | have a great week! | 06:45 |
| dcaliste | Thank you too | 06:45 |
| Venemo | I can take a look at the MR as well, if you want, chriadam | 06:46 |
| chriadam | Venemo: yes, please! | 06:47 |
| chriadam | I havent' had a chance to look at it properly yet, but was planning to, later this week | 06:47 |
| Venemo | okay, I will try | 06:47 |
| dcaliste | Thank you Venemo. | 06:47 |
| Venemo | libcommhistory always brings tears to my eyes | 06:47 |
| *** svartoyg is now known as svartoyg_afk | 07:08 | |
| *** leinir_ is now known as leinir | 07:17 | |
| *** frinring_ is now known as frinring | 08:19 | |
| *** Ischwitch is now known as Ingvix | 15:33 | |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!