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