Code review comment for lp:~gerboland/qtubuntu/exposeOrientation

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

> > Then please use the same nomenclature that the mir event uses in that
> internal
> > OrientationChangeEvent, thus avoiding mentioning landscape or portrait at
> this
> > point, which is misleading. You could stick with the
> > QOrientationReading::Orientation enum.
>
> Can you explain your reasoning why this is misleading? We need to convert from
> Mir event nomenclature (normal,left,right,inverted) to QScreenOrientation
> (portrait, landscape, invertedlandscape, invertedportrait) somewhere, it
> struck me as a good idea to convert to Qt's definition as quickly as possible.

Because in UbuntuInput::dispatchOrientationEvent you're doing a wrong conversion as you're assuming that the native orientation is portrait, just to later fix it in QScreen. By converting it to QOrientationReading you don't have to make any such assumptions. For what it's worth, QOrientationReading is also a "Qt definition".

>
> Do I understand you want me to add an intermediary 3rd nomenclature, the
> QOrientationReading::Orientation. So then we have
> Mir->QOrientationReading->QScreenOrientation ?

Yes. If I understood QOrientationReading correctly, it's a one-to-one mapping of the Mir enum.

But if you don't like that intermediate enum, then I'm fine if you store the Mir enum value in that internal QEvent as well. I just don't want the wrong conversion I mentioned above.

« Back to merge proposal