fix virtual keyboard on Plasma Mobile
Needs RevisionPublic

Authored by mart on Dec 10 2018, 3:45 PM.

Details

Summary

the newly introduced resize button shouldn't be visible on the phone and
for some reason using a ToolButton prevents the qml to load at all on
the phone (the problem seems right into the C++ part of ToolButton as
is broken on every QQC2 style)

Test Plan

work again on phone, still work on laptop

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.
mart created this revision.Dec 10 2018, 3:45 PM
Restricted Application added a project: KWin. · View Herald TranscriptDec 10 2018, 3:45 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
mart requested review of this revision.Dec 10 2018, 3:45 PM
mart added a reviewer: bshah.Dec 10 2018, 3:45 PM
bshah accepted this revision.Dec 10 2018, 3:49 PM
This revision is now accepted and ready to land.Dec 10 2018, 3:49 PM
This revision was automatically updated to reflect the committed changes.
graesslin reopened this revision.Dec 10 2018, 4:50 PM
graesslin added a subscriber: graesslin.

Guys, code review doesn't make sense if one mobile dev opens a change for another mobile dev and the whole thing gets pushed in five minutes. This gives nobody else a chance to comment. If you want to operate like that you can just omit the review.

qml/virtualkeyboard/main.qml
23

This introduces a new dependency on KWin which we otherwise do not use or need. I think it is overkill to use kirigami for this. Please change.

This revision is now accepted and ready to land.Dec 10 2018, 4:50 PM
graesslin requested changes to this revision.Dec 10 2018, 4:50 PM
This revision now requires changes to proceed.Dec 10 2018, 4:50 PM

This introduces a new dependency on KWin

It doesn't.
We already link Plasma in Core, that has already dependency on Kirigami.

bshah added a comment.Dec 10 2018, 5:37 PM

Sorry for this, me and @mart did realize that this was pushed too early. If you prefer, we can revert Kirigami part.

ToolButton code path is broken on mobile and makes it not visible at all so that is needed.

mart added a comment.Dec 10 2018, 7:44 PM

Sorry for this, me and @mart did realize that this was pushed too early. If you prefer, we can revert Kirigami part.

ToolButton code path is broken on mobile and makes it not visible at all so that is needed.

tough also disabling the button, as on the phone besides not making sense, it covers backspace and makes the keyboard functionally unusable

This introduces a new dependency on KWin

It doesn't.
We already link Plasma in Core, that has already dependency on Kirigami.

Libplasma links kirigami?

I think to say hide the button on mobile is the wrong conclusion. You need to hide it because it overlaps. So hide it based on size. That would also be useful on desktop and not only on mobile.

That it is changed to button from toolbutton is something I don't understand at all. If it doesn't work on mobile maybe fix instead of adjusting all uses of toolbutton?