Be consistent about touch point id type: use qint32
ClosedPublic

Authored by gladhorn on Sun, Aug 11, 9:53 AM.

Details

Summary

There is no point in using quint32 and casting back and forth in numerous places.
Fix a bunch of compiler warnings that we implicitly cast between signed and unsigned.
This makes things consistent with what we get from libinput.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
gladhorn created this revision.Sun, Aug 11, 9:53 AM
Restricted Application added a project: KWin. · View Herald TranscriptSun, Aug 11, 9:53 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
gladhorn requested review of this revision.Sun, Aug 11, 9:53 AM

This makes sense because libinput provides (non-negative) int32 ids/slots.

Qt uses qint32

Why and where is this relevant?

zzag added a subscriber: zzag.Sun, Aug 11, 2:04 PM

Someone "fixed" these warnings a while ago, but you reverted the fix.

This makes sense because libinput provides (non-negative) int32 ids/slots.

Even if it makes sense, the warnings have to be fixed. Comparing signed and unsigned numbers without taking extra measures is bad.

In D23086#510314, @zzag wrote:

Someone "fixed" these warnings a while ago, but you reverted the fix.

I know. I reverted because it broke functionality. So there was no fix.

This makes sense because libinput provides (non-negative) int32 ids/slots.

Even if it makes sense, the warnings have to be fixed. Comparing signed and unsigned numbers without taking extra measures is bad.

"This makes sense" as in this fix here. If libinput would provide uint32 ids it would not make sense.

In D23086#510314, @zzag wrote:

Someone "fixed" these warnings a while ago, but you reverted the fix.

I know. I reverted because it broke functionality. So there was no fix.

This makes sense because libinput provides (non-negative) int32 ids/slots.

Even if it makes sense, the warnings have to be fixed. Comparing signed and unsigned numbers without taking extra measures is bad.

"This makes sense" as in this fix here. If libinput would provide uint32 ids it would not make sense.

I though these were Qt touch points, interesting. In any case, I hope this is more complete than just casting in 16e904592ae4fe0a04ef61d42c9a114c62997c8e.
What is the best way to test these? I'll run my desktop with the change. I don't use the touch screen that often though.

...
I though these were Qt touch points, interesting. In any case, I hope this is more complete than just casting in 16e904592ae4fe0a04ef61d42c9a114c62997c8e.
What is the best way to test these? I'll run my desktop with the change. I don't use the touch screen that often though.

Yea, they come from libinput and should only be relevant in Wayland session. Not sure about the effects stuff though. To test simply try touch input in Wayland session on client content and window decorations (tapping, dragging windows). If everything works as expected there should be no problem. I mean on the other side you can spot pretty easily in the reverted patch what the issue was there: m_touchId is sometimes -1 but gets casted to uint in the comparison.

gladhorn updated this revision to Diff 63546.Sun, Aug 11, 2:29 PM
gladhorn retitled this revision from Be consistent about touch point id type: Qt uses qint32 to Be consistent about touch point id type: use qint32.
gladhorn edited the summary of this revision. (Show Details)

Fix up commit message

I played with this after spending some time to get a working wayland session. I could not see any problems, decorations work, the touch point visualization also works.

romangg accepted this revision.Sun, Aug 11, 6:38 PM

Great, thanks. Please make sure arc won't fail on this commit. As a tip: when I'm unsure about it I use arc land with the --holdoption. This way I can inspect afterwards if the commit message and everything is alright before pushing normally with git the resulting commit to the remote.

This revision is now accepted and ready to land.Sun, Aug 11, 6:38 PM
This revision was automatically updated to reflect the committed changes.