Merge lp://staging/~haggai-eran/unity-2d/rtl into lp://staging/unity-2d/3.0

I'm reviewing this merge request, and trying the code rebased on unity-2d/4.0; unfortunately there are merge conflicts for all po/*.po files (which are *a lot*).
Haggai, do I understand correctly that the only change needed in the .po file is the addition of the QT_LAYOUT_DIRECTION id, which must be translated to "RTL" for the right-to-left languages?

Here is the same branch, merged on unity-2d/4.0, without the po file changes:
https://code.launchpad.net/~mardy/unity-2d/rtl

I quickly tested it under the he_IL locale (and adding the QT_LAYOUT_DIRECTION translation as "RTL" in the he.po file), and it seemed to work correctly at a first sight.
Haggai, if you have a change to run Oneiric and test my branch, it will be very much appreciated. Meanwhile, I'll continue testing/reviewing the code in my branch, for the LTR locales, just to make sure that there are no regressions.

In void EdgeHitDetector::updateGeometryFromScreen():

205 + QPoint p = QApplication::isLeftToRight() ?
206 + QPoint() :
207 + QPoint(QApplication::desktop()->width(), 0);

In line 207 I think you should decrement the width by 1 pixel, otherwise you get one pixel which is outside of the desktop area (same applies to the change in panel/app/panelmanager.cpp).
Also, I would save a pointer to QApplication::desktop(), which is used three times in this method.

libunity-2d-private/src/intellihidebehavior.cpp:

241 - launcherRect.moveLeft(0);
242 + switch (static_cast<Unity2dPanel*>(panel())->edge()) {
243 + case Unity2dPanel::LeftEdge:
244 + if (QApplication::isLeftToRight()) {
245 + launcherRect.moveLeft(0);
246 + }
247 + else {
248 + QDesktopWidget* desktop = QApplication::desktop();
249 + const QRect screen = desktop->screenGeometry(m_panel);
250 + launcherRect.moveRight(screen.right());
251 + }
252 + break;
253 + }

Why the "switch"? It could be a simple "if"; anyway, it seems to change the logic for the LTR case too, so I wonder if this is about fixing some other bug too.

In places/app/dashdeclarativeview.cpp:

657 + int screenRight = rect.right();
658
659 rect.setWidth(qMin(DASH_DESKTOP_WIDTH, rect.width()));
660 rect.setHeight(qMin(m_expanded ? DASH_DESKTOP_EXPANDED_HEIGHT : DASH_DESKTOP_COLLAPSED_HEIGHT,
661 rect.height()));
662
663 + if (QApplication::isRightToLeft())
664 + rect.moveRight(screenRight);

You can do without the screenRight variable.

In places/Dash.qml, why do you require QtQuick 1.1? (AFAIK, it's not released yet)

About the other changes in the QML code: they look all fine, but I'd suggest to rename the leftRight() and rightLeft() functions to something more easily understood, something like "ifLeftToRight()" and "ifRightToLeft()", even though I'm not sure I like those either. :-)
Or maybe remove them completely, and use only isLeftToRight() and isRightToLeft() -- which would avoid calling functions with less parameters than declared (which is valid in JS, but the resulting code is not that readable, IMHO).

OTOH, both these methods will probably be no longer necessary with Qt 4.7.4 (see http://doc.qt.nokia.com/4.7-snapshot/qml-righttoleft.html ), so I guess we can live with those names for the time being.

Hi,

Thanks for reviewing my code. I'll try to answer the questions below.

> I'm reviewing this merge request, and trying the code rebased on unity-2d/4.0;
> unfortunately there are merge conflicts for all po/*.po files (which are *a
> lot*).
> Haggai, do I understand correctly that the only change needed in the .po file
> is the addition of the QT_LAYOUT_DIRECTION id, which must be translated to
> "RTL" for the right-to-left languages?

You are correct. I don't know why updating the pot file caused all these changes, but the only thing needed is the QT_LAYOUT_DIRECTION. It follows the Qt documentation.

I'll try to test your branch and see how it works.

> In void EdgeHitDetector::updateGeometryFromScreen():
>
> 205 + QPoint p = QApplication::isLeftToRight() ?
> 206 + QPoint() :
> 207 + QPoint(QApplication::desktop()->width(), 0);
>
> In line 207 I think you should decrement the width by 1 pixel, otherwise you
> get one pixel which is outside of the desktop area (same applies to the change
> in panel/app/panelmanager.cpp).
> Also, I would save a pointer to QApplication::desktop(), which is used three
> times in this method.

I'll change that.

> libunity-2d-private/src/intellihidebehavior.cpp:
>
> 241 - launcherRect.moveLeft(0);
> 242 + switch (static_cast<Unity2dPanel*>(panel())->edge()) {
> 243 + case Unity2dPanel::LeftEdge:
> 244 + if (QApplication::isLeftToRight()) {
> 245 + launcherRect.moveLeft(0);
> 246 + }
> 247 + else {
> 248 + QDesktopWidget* desktop = QApplication::desktop();
> 249 + const QRect screen = desktop->screenGeometry(m_panel);
> 250 + launcherRect.moveRight(screen.right());
> 251 + }
> 252 + break;
> 253 + }
>
> Why the "switch"? It could be a simple "if"; anyway, it seems to change the
> logic for the LTR case too, so I wonder if this is about fixing some other bug
> too.

Well, at first I thought I should add a 'RightEdge' entry to the enum, and that's why the switch was there. I changed my mind later, and thought that it would be best just to change the semantic of LeftEdge when using an RTL locale, but I guess I forgot to delete the switch.
I didn't mean to change the LTR case.

> In places/app/dashdeclarativeview.cpp:
>
> 657 + int screenRight = rect.right();
> 658
> 659 rect.setWidth(qMin(DASH_DESKTOP_WIDTH, rect.width()));
> 660 rect.setHeight(qMin(m_expanded ? DASH_DESKTOP_EXPANDED_HEIGHT :
> DASH_DESKTOP_COLLAPSED_HEIGHT,
> 661 rect.height()));
> 662
> 663 + if (QApplication::isRightToLeft())
> 664 + rect.moveRight(screenRight);
>
> You can do without the screenRight variable.

Shouldn't I save it's value? The width of rect might change at line 659.

> In places/Dash.qml, why do you require QtQuick 1.1? (AFAIK, it's not released
> yet)

I'm so sorry about this. I read at the reference you quote below about QtQuick's coming RTL support, and I tried enabling it, but since I didn't have Qt 4.7.4, it didn't work :)
I forgot to return it to QtQuick 1.0 before committing.

> About the other changes in the QML code: they look all fine, but I'd suggest
> to rename the leftRight() and rightLeft() functions to something more ea...

> I'll try to test your branch and see how it works.

I just updated it; the mirroring of the application title in the panel was not implemented (it was properly implemented in your original patch, but I lost it when porting it to Oneiric).

> Well, at first I thought I should add a 'RightEdge' entry to the enum, and
> that's why the switch was there. I changed my mind later, and thought that it
> would be best just to change the semantic of LeftEdge when using an RTL
> locale, but I guess I forgot to delete the switch.
> I didn't mean to change the LTR case.

You are right. However, since this is not strictly related to the RTL functionaly, I'd leave it without switch, and let's address it in another merge request (in my branch, I added a FIXME comment not to forget about it).

[...]
> > You can do without the screenRight variable.
>
> Shouldn't I save it's value? The width of rect might change at line 659.

My bad, sorry. You are absolutely right.

> I'm so sorry about this. I read at the reference you quote below about
> QtQuick's coming RTL support, and I tried enabling it, but since I didn't have
> Qt 4.7.4, it didn't work :)
> I forgot to return it to QtQuick 1.0 before committing.

No big issue. :-)

[...]
> I think that using such function is preferred to using isRightToLeft, and ?:
> operator, but I guess it's a matter of taste. I don't mind changing the name,
> or using the ?: operator, and I can pass undefined directly instead of relying
> on this JS feature.

Unless others have strong opinions on this, I'd leave the code as you wrote it. At least, I couldn't come up with a more explicative name for leftRight() and rightLeft(), so I kept them like this in my merge request as well.

> But I agree that once we have Qt 4.7.4 the code could be much simpler. I
> couldn't find when it's release is planned, but there's no chance it will make
> it to Oneiric, right?

I think it might still be possible to have it in Oneiric, if it's released soon, because it's most likely totally backward compatible. But I really don't know. :-)

> Should I make the changes against the branch you published, or against this
> branch?

I did the relevant changes to my branch, so for Oneiric we should be fine (it would be great if you could spend some time to review my branch, to make sure that I didn't introduce some bugs or left out some pieces).
You may still want to update this branch, in case there's a chance of an update of unity-2d for Natty (Florian, is there?).

[...]
> > Should I make the changes against the branch you published, or against this
> > branch?
>
> I did the relevant changes to my branch, so for Oneiric we should be fine (it
> would be great if you could spend some time to review my branch, to make sure
> that I didn't introduce some bugs or left out some pieces).
> You may still want to update this branch, in case there's a chance of an
> update of unity-2d for Natty (Florian, is there?).

I will do the appropriate backport to Natty if possible and if necessary.
Haggai, do you need LTR support in the Natty version?

Alberto,
I tried testing your branch, but I had to merge it with 4.0 in order for it to compile on an updated oneiric system. In order for it to work I also had to add the QT_LAYOUT_DIRECTION to the po file. Should I do it manually, without updating the pot file and the rest of the po files?
Another problem is that the new 4.0.0 version stopped using QMenuBar for the menus, and now even in RTL mode, the menus open aligned to the left with the menu title. (The entries inside the menus align to the right just fine, only the menu popup window isn't positioned correctly). I think fixing it will require modifying libunity-core, since (if I understand correctly), it is now responsible for showing the menu.

In addition I noticed that pressing the left/right keyboard arrows when the menu is open in RTL mode, opens the menus in the wrong direction. Do you think changing that will also need to be done in libunity-core?

Florian,
Having RTL support in Natty would be great. I personally plan to upgrade to Oneiric when it's released, but I'm sure people that continue to use Natty will appreciate it.

I wrote some code to solve the problem with the menus, but I had to change nux and unity as well, since unity-service is now responsible for the menus even in unity-2d.

I've put the unity-2d branch at lp:~haggai-eran/unity-2d/4.0-rtl and it depends on lp:~haggai-eran/unity/rtl-menu-popup and lp:~haggai-eran/nux/rtl .

> Alberto,
> I tried testing your branch, but I had to merge it with 4.0 in order for it to
> compile on an updated oneiric system. In order for it to work I also had to
> add the QT_LAYOUT_DIRECTION to the po file. Should I do it manually, without
> updating the pot file and the rest of the po files?

When making the next release we'll update the pot file, and the translators will have to translate the QT_LAYOUT_DIRECTION text as appropriate.
For testing purposes yes, you'll have to update it manually.

For the other issues, looks like you did a great job in fixing them! Thanks!
I think you need to file two different merge requests to get your changes accepted in unity and nux, first.

Okay, I'll file the merge requests. Thanks.

I believe this work has been merged albeit with modifications. Thanks to everybody involved, especially Haggai!