Review Request: Allow user to select Calendar System to display in Calendar and Clock Applets

Aaron Seigo aseigo at kde.org
Mon Aug 24 14:30:34 CEST 2009 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1378/#review2131
-----------------------------------------------------------


generally i like both the concept and the implementation. there are a few comments and some minor adjustments needed before committing, but it's all pretty easy stuff :)


trunk/KDE/kdebase/workspace/libs/plasmaclock/calendar.h
<http://reviewboard.kde.org/r/1378/#comment1462>

    ' = ' (spaces)



trunk/KDE/kdebase/workspace/libs/plasmaclock/calendar.h
<http://reviewboard.kde.org/r/1378/#comment1465>

    why the change to return a bool? just correctness, or? because in this case it is really an implementation detail whether or not a matching data engine was found or not, and it's not an error that is recoverable from (nor one that causes any trouble; if the engine doesn't exist a dummy engine is returned).



trunk/KDE/kdebase/workspace/libs/plasmaclock/calendar.h
<http://reviewboard.kde.org/r/1378/#comment1463>

    why is this marked as a HACK?



trunk/KDE/kdebase/workspace/libs/plasmaclock/calendar.cpp
<http://reviewboard.kde.org/r/1378/#comment1464>

    wouldn't the API be kept smaller and the code using this clearer if they use of Calendar just used the pointer return by calendarTable() directly? otherwise we end up with all this duplicated API and every time CalendarTable changes, this API will need to be adjusted in parallel?



trunk/KDE/kdebase/workspace/libs/plasmaclock/calendarConfig.ui
<http://reviewboard.kde.org/r/1378/#comment1468>

    just: "Calendar System:"?



trunk/KDE/kdebase/workspace/libs/plasmaclock/calendarConfig.ui
<http://reviewboard.kde.org/r/1378/#comment1469>

    instead of this, why not just have "Holidays" (instead of "Use Holiday Region") and have as the first entry "None". "None" would be equivalent to unselecting the "Display Holidays" checkbox, dropping the usage complexity of this page quite a bit.



trunk/KDE/kdebase/workspace/plasma/applets/calendar/calendar.cpp
<http://reviewboard.kde.org/r/1378/#comment1467>

    this is not a public class in a library. there is no point to a private class other than to make backtraces harder to follow. instead, calendarWidget and theme should be members of ClockApplet and prefixed with m_ just as they were prior to your changes.


- Aaron


On 2009-08-20 23:45:23, John Layt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1378/
> -----------------------------------------------------------
> 
> (Updated 2009-08-20 23:45:23)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> [Requires kdelibs revision 1013825]
> 
> Currently the Calendar and Clock applets can only display the global locale Calendar System, usually Gregorian.  However it would be useful for users to be able to display Calendar applets with different Calendar Systems, for example Hebrew or Hijri, similar in concept to being able to have multiple Clock applets showing different Timezones.  This change enables different applets to display different Calendar Systems and allows the user to configure each of these.  See my blog post at http://www.layt.net/john/blog/odysseus/displacement_activity for screen shots.
> 
> This is primarily achieved by moving most of the calendar configuration code from ClockApplet to CalendarTable from where it can be shared by any applet that embeds the CalendarTable.  The Calendar applet has been modified to use the common configuration, as has the base ClockApplet class which ensures all Clock applets do as well. [I've had some thoughts to move the generic code from CalendarTable to a new non-gui helper class so different calendar layout widgets could share it, but that's not needed yet].
> 
> The major implementation issue I have to raise is in the ClockApplet class where I now create the CalendarTable in the init() instead of in initExtenderExtender(), and the extender item now gets passed the already existing CalendarTable which I believe it then manages.  I _think_ this is safe to do as we will only ever have one calendar extender and it never appears to be deleted during the life of the applet, so I don't think the CalendarTable could be accidentally deleted.  This means there is always a calendarWidget and calendar object so the code can be simplified in lots of places to remove checks to see if it exists first.
> 
> This change does not yet address the issue in the Digital Clock applet that the displayed Date format is not correctly localised, but this involves a number of usability issues that I think are best addressed separately.
> 
> Changes:
> 
> ClockApplet:
>   Remove all calendar configuration code and vars, call CalendarTable implementation instead
>   Add convenience method calendar() for shortcut
>   Clean-up clipboardMenu to only have localised date/time formats, add other calendar system options
>   Remove unused method caseInsensitiveLessThan()
> 
> CalendarTable
>   Add all calendar configuration code and vars from ClockApplet
>   Add ability to set and save calendarType, special type of "locale" means use global locale type
>   Improved validation
> 
> Calendar
>   API to pass through calendar config calls to CalendarTable
> 
> generalConfig.ui
>   Removed all calendar configuration options to new calendarConfig.ui
> 
> calendarConfig.ui
>   New, has all calendar configuration options from generalConfig.ui
>   Added combo box to select calendar system from.
> 
> Clock (digital applet)
>   Display date on clock face in correct calendar system 
> 
> CalendarApplet
>   Add d-> private class, use for variables
>   Add new shared calendar config via CalendarTable
>   Localise day number shown on Icon view
> 
> [My apologies if my over-zealous clean-up of code layout and renaming makes the diff a little hard to read in places]
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdebase/workspace/libs/plasmaclock/CMakeLists.txt 1013705 
>   trunk/KDE/kdebase/workspace/libs/plasmaclock/calendar.h 1013705 
>   trunk/KDE/kdebase/workspace/libs/plasmaclock/calendar.cpp 1013705 
>   trunk/KDE/kdebase/workspace/libs/plasmaclock/calendarConfig.ui PRE-CREATION 
>   trunk/KDE/kdebase/workspace/libs/plasmaclock/calendartable.h 1013705 
>   trunk/KDE/kdebase/workspace/libs/plasmaclock/calendartable.cpp 1013705 
>   trunk/KDE/kdebase/workspace/libs/plasmaclock/clockapplet.h 1013705 
>   trunk/KDE/kdebase/workspace/libs/plasmaclock/clockapplet.cpp 1013705 
>   trunk/KDE/kdebase/workspace/libs/plasmaclock/generalConfig.ui 1013705 
>   trunk/KDE/kdebase/workspace/plasma/applets/calendar/calendar.h 1013705 
>   trunk/KDE/kdebase/workspace/plasma/applets/calendar/calendar.cpp 1013705 
>   trunk/KDE/kdebase/workspace/plasma/applets/digital-clock/clock.cpp 1013705 
> 
> Diff: http://reviewboard.kde.org/r/1378/diff
> 
> 
> Testing
> -------
> 
> Used plasmoidviewer to add/remove/play with lots of Calendar and Clock applets.
> 
> 
> Thanks,
> 
> John
> 
>

More information about the Plasma-devel mailing list