Code review comment for lp://staging/~ubuntu-sdk-team/ubuntu-ui-toolkit/dpr

> Good stuff. Here is a bunch of questions and remarks.
>
> 1)
> In the shape, storing the radius as device-independent pixel like that
>
> - float radius = UCUnits::instance().gridUnit() * (m_radius == Small ?
> smallRadiusGU : mediumRadiusGU);
> - const float scaledDownRadius = qMin(itemSize.width(), itemSize.height())
> * dpr * 0.5f * 0.8f;
> + float radius = UCUnits::instance().gu((m_radius == Small ? smallRadiusGU
> : mediumRadiusGU));
> + const float scaledDownRadius = qMin(itemSize.width(), itemSize.height())
> * 0.5f * 0.8f;
>
> would remove the need to multiply the item size by the dpr for the shape
> coordinates attributes in updateVertices() (for both UbuntuShape and
> UbuntuShapeOverlay). That would require to do change updateMaterial() though,
> but in a less intrusive way IMO:
<snip>
Done!

> 2) The previous change would allow to move the devicePixelRatio retrieval in
> updateGeometryNode() to the "if (m_flags & DirtySourceTransform) {" scope.
Done

> 3)
> - Q_INVOKABLE float dp(float value);
> - Q_INVOKABLE float gu(float value);
> + Q_INVOKABLE float dp(const float value);
> + Q_INVOKABLE float gu(const float value);
>
> Using const for value parameters (not for reference or pointer parameters)
> isn't a common practice in the toolkit (neither in Qt AFAIK). Is there a
> particular reason for it?
I had thought compiler could optimize a little better with this, but I'm told it's of no use. Reverted.

> 4) Wouldn't it be better (faster) to inline UCUnits::devicePixelRatio()
> definition in the header?
>
> 5) Talking about UCUnits::devicePixelRatio(), why do we have to expose it
> instead of using QGuiApplication::devicePixelRation() or
> QWindow::devicePixelRatio() directly? Same question for the "manual" retrieval
> of QT_DEVICE_PIXEL_RATIO in UCUnits constructor.

Yeah I got rid of this. I had mainly added it to UCUnits to ease mocking it for testing. Instead I've added a custom minimal QPA plugin to mock the value of devicePixelRatio(). So UCUnits API has be left unchanged by this MR.

>
> 6) UCUnits::gu() and UCUnits:dp() returns "qRound(value * m_gridUnit) /
> m_devicePixelRatio", I'm wondering if the division should be applied before
> the rounding?

I left the division after the rounding as Qt will take this value, multiply it by devicePixelRatio to calculate the physical pixels. I.e.
physicalPixels = [qRound(value * m_gridUnit) / m_devicePixelRatio] * m_devicePixelRatio
should be more correct than
physicalPixels = qRound(value * m_gridUnit / m_devicePixelRatio) * m_devicePixelRatio

> 7) It took me some time to figure out the logic because I didn't realize we
> had to support QtWidgets as well. I think we'd better explain the whole
> decision somewhere in the project, either in the README or in the UCUnits
> module. I'd tend to go for a good explanation in the ucunits.cpp module
> documentation maybe adding toolkit implementation details in there in a "non-
> exposed" comment if needed. We should also agree on a better naming for the
> units, currently there are grid units, device pixels, value density
> independent pixels, actual pixels, physical pixels, etc, it's very confusing.
> In Qt they use device-independent pixels and physical pixels, we should try to
> follow that convention if possible (device-independent is better than device
> because the latter one could also be considered as physical). But maybe that's
> something for another MR? I can take care of it if you want.

Fair point. I've added a preliminary comment on the motivation and consolidated the terminology a bit. I don't think it of use to mention density-independent pixels to app devs using the UITK, most shouldn't have to care. Your call tho.