Split the startDate property into today and displayedDate in calendar component

Review Request #118668 - Created June 11, 2014 and submitted

Information
Martin Klapetek
plasma-framework
Reviewers
plasma
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.
All works properly

Issues

  • 1
  • 0
  • 0
  • 1
Description From Last Updated
Don't forget to remove these debugs before committing. Sebastian Kügler Sebastian Kügler
Sebastian Kügler
Commit Hook
This review has been submitted with commit 3188b417f4b898d416646e3c46999d710650bf3d by Martin Klapetek to branch master.
Martin Klapetek
Review request changed

Status: Closed (submitted)

Mark Gaiser
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 :)
  1. 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 :)
  2. +1, thank you very much for the explanation.
Loading...