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