Review Request 118668: Split the startDate property into today and displayedDate in calendar component

[Mark Gaiser]

> On June 17, 2014, 6:16 p.m., Mark Gaiser wrote:
> > No comments on most of the code change since that looks OK to me :) Besides setToday.
> > Just a friendly explanation why i did it this way and why i (back then) thought it would be sufficient.
> > 
> > I knew there was going to be a usecase where users of the class would want to know the current day, would want to change it and move back to it. My reasoning for not implementing functionality for that is because startDate could handle it all.
> > - Want to jump to a date? startDate = new Date(YYYY, MM, DD)
> > - Want to jump back to the current date? startDate = new Date()
> > - Want to know the current day? new Date() :)
> > 
> > But i do see now that my naming back then (startDate) is confusing when it allows you to do multiple things. I'm sorry for the hassle you must have went through.
> > 
> > Lastly setToday. I don't understand that one. today() should always return a QDate::currentDate(), right? Why should there be a setter for it? If i understand it correctly you only need this because of the dataengine providing date updates, right? If that's the case then i don't see why i needs to be in this class.. It should be in the QML side as a custom property like so:
> > 
> > Calendar {
> >   id: cal
> >   property date dateEngineDate: somethingFromTheDataEngine
> >   onDateEngineDateChanged: {
> >     // do your magic.. Probably the most part of your current setToday function
> >   }
> > }
> > 
> > Or i am completely wrong, quite possibly :)
> 
> Martin Klapetek wrote:
>     Yeah, the setToday is for when the date changes and/or initializing the date for today. It's better to have one value cached than keeping recreating the date object all the time and pass it back and forth between qml and qt. It's also useful for timezones when eg. your timezone has date 21st but GMT has still 20th. If we're going to add multiple timezone clocks back to the clock applet, you'll need to set different 'today' when you switch the clock. Finally, direct binding for the 'today' means less glue code all around. It's just simpler :)

+1, thank you very much for the explanation.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118668/#review60307
-----------------------------------------------------------


On June 17, 2014, 8:44 a.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118668/
> -----------------------------------------------------------
> 
> (Updated June 17, 2014, 8:44 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> -------
> 
> Basically splits the Calendar::m_startDate into 'today' and 'displayedDate', where displayedDate is the date that is displayed (it controls the days model etc) and can be manipulated by the user by eg. changing months in the plasmoid, and today is the current day, populated by our dataengine (which means it auto-updates with no need for a timer). This allows for greater flexibility and things like "Go back to today" when eg. the plasmoid is hidden or when the user have browsed too far in the calendar and just wants to get back to today (the button to do that pending). Also this fixes a problem where the time dataengine is being polled every 30secs for the clock and would reset the calendar view as the startDate is currently bound to the dataengine and the view resets on that change.
> 
> 
> Diffs
> -----
> 
>   src/declarativeimports/calendar/calendar.h fd2c534 
>   src/declarativeimports/calendar/calendar.cpp 4225579 
>   src/declarativeimports/calendar/qml/MonthMenu.qml 89e9dc2 
>   src/declarativeimports/calendar/qml/MonthView.qml eee850d 
> 
> Diff: https://git.reviewboard.kde.org/r/118668/diff/
> 
> 
> Testing
> -------
> 
> All works properly
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140617/289ac039/attachment.html>