Code review comment for lp:~haggai-eran/unity-2d/rtl

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

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

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