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

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