'Re: Review Request: Allow user to select Calendar System to display in'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: Review Request: Allow user to select Calendar System to display in
From:       "Aaron Seigo" <aseigo () kde ! org>
Date:       2009-08-24 12:30:34
Message-ID: 20090824123034.29554.88519 () localhost
[Download RAW message or body]

-----------------------------------------------------------
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
> 
> 

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

[prev in list] [next in list] [prev in thread] [next in thread] 