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

List:       kde-panel-devel
Subject:    Re: Review Request: Fix Plasma CalendarTable widget for RTL
From:       "John Layt" <johnlayt () googlemail ! com>
Date:       2009-11-29 0:56:27
Message-ID: 20091129005627.13243.24664 () localhost
[Download RAW message or body]


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

(Updated 2009-11-29 00:56:27.688430)


Review request for Plasma.


Changes
-------

While trying to fix a bug in the behaviour of the original patch, I thought \
about this some more, and decided that rather than patch over the top I \
should really fix the problem at it's root, so here's a revised version \
that separates the date offset calculations from the screen drawing \
position.

1) There's far fewer places that need to check for RTL
2) The code is simpler and cleaner
3) This also solves the bug where the hover highlight usually doesn't \
disappear when moving the mouse off the calendar, and where borders got \
confused (well almost, moving off the right edge doesn't remove the hover \
at first, but if you resize the widget it does start working, very \
mysterious). 4) It gets rid of the localeDateNum() stuff by using the \
KCalendarSystem string methods (changes made in kdelibs for this). 5) More \
clean-ups and some krazy fixes.


Summary
-------

The CalendarTable widget does not correctly display in RTL mode, the cells \
in the table go LTR instead.  The numbers in the cells are controlled by \
code and not the widget layout, so we need to handle this the code.  

This patch fixes the day numbers, weekday names, and week numbers to go \
RTL, see screenshots for example.  It doesn't move the Week Numbers column \
from the left to the right as this is part of the SVG, we would need to \
flip the SVG along it's vertical axis to do so and shuffle everything \
about.  Is this possible or too much effort?  

Also rename a couple of variables for clarity seeing as I now understand \
what they do.

Patch against trunk, I'll backport to 4.3 if all OK.  Note we may want to \
rewrite some of the CalendarTable in 4.5 for a cleaner implementation.

cc to Dotan to confirm this looks OK and to confirm it's not a problem if \
we can't move the Week Number column.


This addresses bug 182976.
    https://bugs.kde.org/show_bug.cgi?id=182976


Diffs (updated)
-----

  trunk/KDE/kdebase/workspace/libs/plasmaclock/calendartable.cpp 1055882 

Diff: http://reviewboard.kde.org/r/2088/diff


Testing
-------

Tested using 'plasmoidviewer --reverse calendar'


Screenshots
-----------

Left to Right mode
  http://reviewboard.kde.org/r/2088/s/253/
Right to Left mode
  http://reviewboard.kde.org/r/2088/s/254/


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]