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

Revision history for this message
Alberto Mardegan (mardy) wrote :

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.

« Back to merge proposal