Re: Review Request: Adjust KDateTimes to Current TimeSpec of the Calendar
> On Nov. 9, 2011, 10:33 a.m., David Narváez wrote: > > If there are no more comments against or in favor of this patch, I'm > > assuming everyone is OK with it so I'll commit this patch. > > Sergio Luis Martins wrote: > Ok. > > Please add a note in the TODO that we'll need to look at this again when > using the kdepimlibs library. TBH, I'd rather not change anything in the Akonadi code that causes it to be out of sync with the kdepim code, as that will just make keeping them in sync and applying patches from kdepim harder to do. It also adds more work later when switching over to the kdepimlibs version. I think this belongs instead in the Data Engine. The Data Engine should be the one making any required adjustments to translate between the data source and the applet request. We can't expect the average applet to know to deal with the issue correctly, that's exactly what the data engine is for. - John --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102997/#review8038 --- On Nov. 8, 2011, 9:14 a.m., David Narváez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102997/ > --- > > (Updated Nov. 8, 2011, 9:14 a.m.) > > > Review request for Plasma and Sergio Luis Martins. > > > Description > --- > > Adjust KDateTimes after finding out the type of incidence added. Also adjust > KDateTimes after a change in the Calendar TimeSpec. > > > This addresses bug 279427. > http://bugs.kde.org/show_bug.cgi?id=279427 > > > Diffs > - > > plasma/generic/dataengines/calendar/akonadi/calendar.cpp 67c12e9 > > Diff: http://git.reviewboard.kde.org/r/102997/diff/diff > > > Testing > --- > > 1. Add an event in any timezone distinct from the local timezone (you can do > that in KOrganizer) > 2. Check the start and end times of the event in the calendar > > Not sure how to test changing timezones from Plasma, so proposed patch is > based on what I think we should do in such a case. > > > Thanks, > > David Narváez > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Adjust KDateTimes to Current TimeSpec of the Calendar
> On Nov. 9, 2011, 10:33 a.m., David Narváez wrote: > > If there are no more comments against or in favor of this patch, I'm > > assuming everyone is OK with it so I'll commit this patch. Ok. Please add a note in the TODO that we'll need to look at this again when using the kdepimlibs library. - Sergio Luis --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102997/#review8038 --- On Nov. 8, 2011, 9:14 a.m., David Narváez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102997/ > --- > > (Updated Nov. 8, 2011, 9:14 a.m.) > > > Review request for Plasma and Sergio Luis Martins. > > > Description > --- > > Adjust KDateTimes after finding out the type of incidence added. Also adjust > KDateTimes after a change in the Calendar TimeSpec. > > > This addresses bug 279427. > http://bugs.kde.org/show_bug.cgi?id=279427 > > > Diffs > - > > plasma/generic/dataengines/calendar/akonadi/calendar.cpp 67c12e9 > > Diff: http://git.reviewboard.kde.org/r/102997/diff/diff > > > Testing > --- > > 1. Add an event in any timezone distinct from the local timezone (you can do > that in KOrganizer) > 2. Check the start and end times of the event in the calendar > > Not sure how to test changing timezones from Plasma, so proposed patch is > based on what I think we should do in such a case. > > > Thanks, > > David Narváez > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Adjust KDateTimes to Current TimeSpec of the Calendar
> On Nov. 1, 2011, 7:39 p.m., Sergio Luis Martins wrote: > > A Calendar object is supposed to mirror what's in akonadi, so we can't > > change Incidence's timezones in there. > > > > I think this should be resolved in the presentation layer, converting to > > the local timezone there ( KSystemTimeZones::local() ) > > David Narváez wrote: > What would be the purpose of sending a TimeSpec to the CalendarModel then? > > David Narváez wrote: > To pick up on this review again, notice that moving the timezone logic to > the Calendar Applet (the presentation layer) implies changing the whole > concept of the Calendar Data Engine. Currently, you can fetch events from > date A to date B using queries to the Calendar Data Engine, but if all > timezone information is handled from the presentation side, then you'd need > to query with time shifts (e.g., in my country where we are at GMT -5, I'd > need to ask for events between 5:00 am today and 5:00 am tomorrow to have my > events for the day). > > A change of this size would requiere a large discussion. As the proposed > patch already deals with code meant to be migrated, and it fixes a bug, I > propose we merge this patch and then rethink this presentation/model issue. > I'd be very happy to help redesign these parts, but I'd be even happier to > close the bug in question. My guess is that they are leftovers from the time when libkcal had both a KDateTime and a QDateTime interface. Methods receiving a QDateTime needed to use a timespec. - Sergio Luis --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102997/#review7834 --- On Nov. 8, 2011, 9:14 a.m., David Narváez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102997/ > --- > > (Updated Nov. 8, 2011, 9:14 a.m.) > > > Review request for Plasma and Sergio Luis Martins. > > > Description > --- > > Adjust KDateTimes after finding out the type of incidence added. Also adjust > KDateTimes after a change in the Calendar TimeSpec. > > > This addresses bug 279427. > http://bugs.kde.org/show_bug.cgi?id=279427 > > > Diffs > - > > plasma/generic/dataengines/calendar/akonadi/calendar.cpp 67c12e9 > > Diff: http://git.reviewboard.kde.org/r/102997/diff/diff > > > Testing > --- > > 1. Add an event in any timezone distinct from the local timezone (you can do > that in KOrganizer) > 2. Check the start and end times of the event in the calendar > > Not sure how to test changing timezones from Plasma, so proposed patch is > based on what I think we should do in such a case. > > > Thanks, > > David Narváez > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Adjust KDateTimes to Current TimeSpec of the Calendar
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102997/#review8038 --- If there are no more comments against or in favor of this patch, I'm assuming everyone is OK with it so I'll commit this patch. - David Narváez On Nov. 8, 2011, 9:14 a.m., David Narváez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102997/ > --- > > (Updated Nov. 8, 2011, 9:14 a.m.) > > > Review request for Plasma and Sergio Luis Martins. > > > Description > --- > > Adjust KDateTimes after finding out the type of incidence added. Also adjust > KDateTimes after a change in the Calendar TimeSpec. > > > This addresses bug 279427. > http://bugs.kde.org/show_bug.cgi?id=279427 > > > Diffs > - > > plasma/generic/dataengines/calendar/akonadi/calendar.cpp 67c12e9 > > Diff: http://git.reviewboard.kde.org/r/102997/diff/diff > > > Testing > --- > > 1. Add an event in any timezone distinct from the local timezone (you can do > that in KOrganizer) > 2. Check the start and end times of the event in the calendar > > Not sure how to test changing timezones from Plasma, so proposed patch is > based on what I think we should do in such a case. > > > Thanks, > > David Narváez > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Adjust KDateTimes to Current TimeSpec of the Calendar
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102997/ --- (Updated Nov. 8, 2011, 9:14 a.m.) Review request for Plasma and Sergio Luis Martins. Changes --- Updating diff against latest changes in master. Description --- Adjust KDateTimes after finding out the type of incidence added. Also adjust KDateTimes after a change in the Calendar TimeSpec. This addresses bug 279427. http://bugs.kde.org/show_bug.cgi?id=279427 Diffs (updated) - plasma/generic/dataengines/calendar/akonadi/calendar.cpp 67c12e9 Diff: http://git.reviewboard.kde.org/r/102997/diff/diff Testing --- 1. Add an event in any timezone distinct from the local timezone (you can do that in KOrganizer) 2. Check the start and end times of the event in the calendar Not sure how to test changing timezones from Plasma, so proposed patch is based on what I think we should do in such a case. Thanks, David Narváez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Adjust KDateTimes to Current TimeSpec of the Calendar
> On Nov. 1, 2011, 7:39 p.m., Sergio Luis Martins wrote: > > A Calendar object is supposed to mirror what's in akonadi, so we can't > > change Incidence's timezones in there. > > > > I think this should be resolved in the presentation layer, converting to > > the local timezone there ( KSystemTimeZones::local() ) > > David Narváez wrote: > What would be the purpose of sending a TimeSpec to the CalendarModel then? To pick up on this review again, notice that moving the timezone logic to the Calendar Applet (the presentation layer) implies changing the whole concept of the Calendar Data Engine. Currently, you can fetch events from date A to date B using queries to the Calendar Data Engine, but if all timezone information is handled from the presentation side, then you'd need to query with time shifts (e.g., in my country where we are at GMT -5, I'd need to ask for events between 5:00 am today and 5:00 am tomorrow to have my events for the day). A change of this size would requiere a large discussion. As the proposed patch already deals with code meant to be migrated, and it fixes a bug, I propose we merge this patch and then rethink this presentation/model issue. I'd be very happy to help redesign these parts, but I'd be even happier to close the bug in question. - David --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102997/#review7834 --- On Oct. 31, 2011, 6:57 a.m., David Narváez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102997/ > --- > > (Updated Oct. 31, 2011, 6:57 a.m.) > > > Review request for Plasma and Sergio Luis Martins. > > > Description > --- > > Adjust KDateTimes after finding out the type of incidence added. Also adjust > KDateTimes after a change in the Calendar TimeSpec. > > > This addresses bug 279427. > http://bugs.kde.org/show_bug.cgi?id=279427 > > > Diffs > - > > plasma/generic/dataengines/calendar/akonadi/calendar.cpp cd96954 > > Diff: http://git.reviewboard.kde.org/r/102997/diff/diff > > > Testing > --- > > 1. Add an event in any timezone distinct from the local timezone (you can do > that in KOrganizer) > 2. Check the start and end times of the event in the calendar > > Not sure how to test changing timezones from Plasma, so proposed patch is > based on what I think we should do in such a case. > > > Thanks, > > David Narváez > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Adjust KDateTimes to Current TimeSpec of the Calendar
> On Nov. 1, 2011, 7:39 p.m., Sergio Luis Martins wrote: > > A Calendar object is supposed to mirror what's in akonadi, so we can't > > change Incidence's timezones in there. > > > > I think this should be resolved in the presentation layer, converting to > > the local timezone there ( KSystemTimeZones::local() ) What would be the purpose of sending a TimeSpec to the CalendarModel then? - David --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102997/#review7834 --- On Oct. 31, 2011, 6:57 a.m., David Narváez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102997/ > --- > > (Updated Oct. 31, 2011, 6:57 a.m.) > > > Review request for Plasma and Sergio Luis Martins. > > > Description > --- > > Adjust KDateTimes after finding out the type of incidence added. Also adjust > KDateTimes after a change in the Calendar TimeSpec. > > > This addresses bug 279427. > http://bugs.kde.org/show_bug.cgi?id=279427 > > > Diffs > - > > plasma/generic/dataengines/calendar/akonadi/calendar.cpp cd96954 > > Diff: http://git.reviewboard.kde.org/r/102997/diff/diff > > > Testing > --- > > 1. Add an event in any timezone distinct from the local timezone (you can do > that in KOrganizer) > 2. Check the start and end times of the event in the calendar > > Not sure how to test changing timezones from Plasma, so proposed patch is > based on what I think we should do in such a case. > > > Thanks, > > David Narváez > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Adjust KDateTimes to Current TimeSpec of the Calendar
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102997/#review7834 --- A Calendar object is supposed to mirror what's in akonadi, so we can't change Incidence's timezones in there. I think this should be resolved in the presentation layer, converting to the local timezone there ( KSystemTimeZones::local() ) - Sergio Luis Martins On Oct. 31, 2011, 6:57 a.m., David Narváez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102997/ > --- > > (Updated Oct. 31, 2011, 6:57 a.m.) > > > Review request for Plasma and Sergio Luis Martins. > > > Description > --- > > Adjust KDateTimes after finding out the type of incidence added. Also adjust > KDateTimes after a change in the Calendar TimeSpec. > > > This addresses bug 279427. > http://bugs.kde.org/show_bug.cgi?id=279427 > > > Diffs > - > > plasma/generic/dataengines/calendar/akonadi/calendar.cpp cd96954 > > Diff: http://git.reviewboard.kde.org/r/102997/diff/diff > > > Testing > --- > > 1. Add an event in any timezone distinct from the local timezone (you can do > that in KOrganizer) > 2. Check the start and end times of the event in the calendar > > Not sure how to test changing timezones from Plasma, so proposed patch is > based on what I think we should do in such a case. > > > Thanks, > > David Narváez > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Adjust KDateTimes to Current TimeSpec of the Calendar
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102997/ --- (Updated Oct. 31, 2011, 6:57 a.m.) Review request for Plasma and Sergio Luis Martins. Changes --- Had to modify the last chunk, the original one didn't compile. On working on this, I noticed the notifyIncidenceChanged method had a deprecation notice that appeared many commits ago and no explanation of what should be used now, so I'd like to read opinions on what do do there. I'll be investigating too. Description --- Adjust KDateTimes after finding out the type of incidence added. Also adjust KDateTimes after a change in the Calendar TimeSpec. This addresses bug 279427. http://bugs.kde.org/show_bug.cgi?id=279427 Diffs (updated) - plasma/generic/dataengines/calendar/akonadi/calendar.cpp cd96954 Diff: http://git.reviewboard.kde.org/r/102997/diff/diff Testing --- 1. Add an event in any timezone distinct from the local timezone (you can do that in KOrganizer) 2. Check the start and end times of the event in the calendar Not sure how to test changing timezones from Plasma, so proposed patch is based on what I think we should do in such a case. Thanks, David Narváez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Adjust KDateTimes to Current TimeSpec of the Calendar
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102997/#review7757 --- Ship it! Ship It! - Aaron J. Seigo On Oct. 30, 2011, 3:31 a.m., David Narváez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102997/ > --- > > (Updated Oct. 30, 2011, 3:31 a.m.) > > > Review request for Plasma and Sergio Luis Martins. > > > Description > --- > > Adjust KDateTimes after finding out the type of incidence added. Also adjust > KDateTimes after a change in the Calendar TimeSpec. > > > This addresses bug 279427. > http://bugs.kde.org/show_bug.cgi?id=279427 > > > Diffs > - > > plasma/generic/dataengines/calendar/akonadi/calendar.cpp 25ec71b > > Diff: http://git.reviewboard.kde.org/r/102997/diff/diff > > > Testing > --- > > 1. Add an event in any timezone distinct from the local timezone (you can do > that in KOrganizer) > 2. Check the start and end times of the event in the calendar > > Not sure how to test changing timezones from Plasma, so proposed patch is > based on what I think we should do in such a case. > > > Thanks, > > David Narváez > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Adjust KDateTimes to Current TimeSpec of the Calendar
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102997/ --- Review request for Plasma and Sergio Luis Martins. Description --- Adjust KDateTimes after finding out the type of incidence added. Also adjust KDateTimes after a change in the Calendar TimeSpec. This addresses bug 279427. http://bugs.kde.org/show_bug.cgi?id=279427 Diffs - plasma/generic/dataengines/calendar/akonadi/calendar.cpp 25ec71b Diff: http://git.reviewboard.kde.org/r/102997/diff/diff Testing --- 1. Add an event in any timezone distinct from the local timezone (you can do that in KOrganizer) 2. Check the start and end times of the event in the calendar Not sure how to test changing timezones from Plasma, so proposed patch is based on what I think we should do in such a case. Thanks, David Narváez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
