virtualkeyboard: resize the focused window to make room for the keyboard
ClosedPublic

Authored by mart on Feb 7 2019, 4:55 PM.

Details

Summary

alternative approach: try to resize the winidow to make room for the keyboard.
the new input wayland protocol doesn't have anymore the overlap rectangle (and it would not be going to work with qwidget apps anyways)

in the future will probably be needed anextension to the input protocol v3 which partially gets back this, tough window resizing is needed regardless

what's missing: the resize should be "temporary" and the window should be restored to its previous geometry when the keyboard closes

Test Plan

tested with test QML code

Diff Detail

Repository
R108 KWin
Branch
arcpatch-D18818
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9318
Build 9336: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
mart added a comment.Feb 7 2019, 5:44 PM
  • remove wrong line
mart abandoned this revision.Feb 7 2019, 5:44 PM
mart reclaimed this revision.
mart updated this revision to Diff 51119.Feb 7 2019, 6:09 PM
  • use geometryRestore
mart updated this revision to Diff 51125.Feb 7 2019, 7:07 PM
  • move the window at the first keyboard show
mart updated this revision to Diff 51134.Feb 7 2019, 9:53 PM
  • correctly restore
romangg added inline comments.Feb 8 2019, 10:45 AM
virtualkeyboard.cpp
237

Can you put this and the next condition in the beginning and:

if(!condition) {
    return;
}
mart updated this revision to Diff 51179.Feb 8 2019, 1:26 PM
  • correctly handle restoring
mart updated this revision to Diff 51181.Feb 8 2019, 1:37 PM
  • clean logic with early returns
mart marked an inline comment as done.Feb 9 2019, 9:36 AM
mart updated this revision to Diff 51240.Feb 9 2019, 9:48 AM
  • remove useless includes
romangg accepted this revision.Feb 9 2019, 10:09 AM

There are some open questions still:

  • How to deal with desktop containments in a sensible way. With this patch applets and the launcher are moved above the virtual keyboard, which is already a great improvement. But we might want to deal with them in a more nicer way.
  • Doe we want to expose the virtual keyboard position information somehow to the clients? The old text-input v2 protocol provided this information. We might want to let clients opt-in to not being size-changed, but placing the keyboard into their window instead at some proposed coordinates. I could imagine this could be nice on larger screens with multiple windows open. But this would need a text-input v4 protocol version or a separate protocol.
  • Talking about multiple windows open: currently only the input focus window is moved. What's with other windows? I might be interested in them as well on a large screen while typing into the focused window.

But anyway from the demo I saw this patch makes the virtual keyboard way more usable. So let's merge it and think about the open questions in the future again.

virtualkeyboard.h
30

Necessary?

This revision is now accepted and ready to land.Feb 9 2019, 10:09 AM
graesslin requested changes to this revision.Feb 10 2019, 4:14 PM

I'm not sure whether it's a good idea to resize windows for virtual keyboard. This can easily result in a feedback loop - the window resizes, the element with focus loses focus, keyboard closes, window resizes, element gets focus again and so on.

I would prefer if we could just move the window away. This could be done just by the compositor.

I'm setting to requires revision as I think from KWin point of view it's wrong to track the geometry for restore in VirtualKeyboard. For everything else the AbstractClient tracks it itself. E.g. there is a restoredGeometry for maximize and for fullscreen. I think the same should be done for virtual keyboard. This also ensures that the system is properly able to track state. The code now is dangerous. If the window is in maximized state, it will lose that state on resize and then the restore geometry gets broken.

This revision now requires changes to proceed.Feb 10 2019, 4:14 PM

Please don't take it negative that I once again request changes here. I once run into this issue myself when I added quick tiling - oh that was a mess, because I didn't track in Client the state changes. Back then Thomas fixed all of it :-) It's just a lesson learned and this geometry handling is really complex. It's too many states and we need to ensure here that we don't jump out of maximized or fullscreen due to keyboard open. Now to make it even more complex one could think about the display getting rotated which would again change the geometries and all the windows changes.

I'll outline an idea how this could work: When virtual keyboard opens the focused window gets maximized and the virtual keyboard acts like a panel with strut - but just for the active window. Then we could use existing KWin functionality without having to track the geometry again.

I'll outline an idea how this could work: When virtual keyboard opens the focused window gets maximized and the virtual keyboard acts like a panel with strut - but just for the active window. Then we could use existing KWin functionality without having to track the geometry again.

On large enough screens (desktop, tablet) we want real window management or tiling and user might want to have a secondary window open for information. For example typing into text editor a summary of a page opened in browser. Just maximizing the active window wouldn't allow that.

Can you expand on the "like a panel with strut" part?

mart added a comment.Feb 11 2019, 1:34 PM

On large enough screens (desktop, tablet) we want real window management or tiling and user might want to have a secondary window open for information. For example typing into text editor a summary of a page opened in browser. Just maximizing the active window wouldn't allow that.

Can you expand on the "like a panel with strut" part?

tough it could maximize only vertically...

mart added a comment.Feb 11 2019, 1:39 PM

Please don't take it negative that I once again request changes here. I once run into this issue myself when I added quick tiling - oh that was a mess, because I didn't track in Client the state changes. Back then Thomas fixed all of it :-) It's just a lesson learned and this geometry handling is really complex.

none taken :) the more i look at this problem, the more complex and full of corner cases it looks like.

It's too many states and we need to ensure here that we don't jump out of maximized or fullscreen due to keyboard open. Now to make it even more complex one could think about the display getting rotated which would again change the geometries and all the windows changes.

I'll outline an idea how this could work: When virtual keyboard opens the focused window gets maximized and the virtual keyboard acts like a panel with strut - but just for the active window. Then we could use existing KWin functionality without having to track the geometry again.

I like that idea (with the caveat roman said, to perhaps maximize only vertically, but i don't have super strong opinions on this)

Implementation wise:

  • would all be done here still saving the old maximized state saved and restored from virtualkeyboard.cpp,
  • or having abstractclient itself knowing when there is a keyboard open and maximize itself when it has focus?

(or something else entirely?)

mart added a comment.Feb 11 2019, 1:50 PM

I'm not sure whether it's a good idea to resize windows for virtual keyboard. This can easily result in a feedback loop - the window resizes, the element with focus loses focus, keyboard closes, window resizes, element gets focus again and so on.

I would prefer if we could just move the window away. This could be done just by the compositor.

you mean the window just to be painted as translated upwards, with parts of the contents outside the top of the screen?

Android has a possible virtual keyboard mode that does that, but is very rarely used in apps since it doesn't work that well and usability wise is kindof annoying.
getting back on Android, just to see what others do, it has 3 possible modes which the app can set in its manifest.xml (and never change again at runtime afaik)

  • sliding up the window enough to show the input field, as i said doesn't really seem to be used buch
  • window resize, the window is resized to make space for the keyboard, all the content that fits is still shown
  • the app makes room for the keyboard in itself: this is pretty much the approach of the previous patch which sets theoverlap region. Unfortunately this was removed from the last wayland input protocol revision, which is unfortunate (i think there are still use cases for it, even tough the security reasons used as motivation kinda make sense, knowing the screen size, its own size and the default keyboard size on the platform, a surface can infer its screen position with a good approximation)

in the end we will probably need some mechanism to select between the different approaches, some or all of the above...
at first tough i would like an approach which works in most of the cases (except on things like the desktop window) which shrinks the window.. if you think that setting a maximized state on the window is safer than just resizing, i can look into that.

mart updated this revision to Diff 52222.Feb 21 2019, 5:33 PM
  • maximize the window when the virtual keyboard is open
mart updated this revision to Diff 52226.Feb 21 2019, 6:02 PM
  • make sire bottom of app os aligned with top of keyboard
mart added a comment.Feb 21 2019, 6:03 PM

so, as an attempt on the "maximizing" suggestion, sets the client to MaximizeFull" and sets a rect of the keyboard, which would act kind of a strut when maximized.
Ideally, it should maximize only vertically, but that's not supported on wayland yet, so that's for another day

doing that, i seen that in changeMaximize it was always unconditionally saving restoreGeometry even if was already maximizeFull, which seemed wrong (and did make the patch not work) i think caused some oddness also during panel resizing

mart added a comment.Feb 27 2019, 11:09 AM

@graesslin is that an approach on the right direction? what does it still need?

[Wednesday, 27 February 2019] [16:40:39 GMT] <notmart> Android has actually 3 modes: resize the window, just tell the app of the area and shift the whole window up
[Wednesday, 27 February 2019] [16:40:49 GMT] <notmart> which is defined by the app

I think this makes a lot of sense, so whilst I don't think this solves everything it seems a sensible first step.
+1 to the concept of translating and resizing.

I'm not super convinced about using maximise:
What if I'm typing a password in the network manager prompt in the plasma applet? or krunner? Does it look weird?

It needs a unit test.

shell_client.cpp
904

why this?

virtualkeyboard.cpp
156

m_trackedClient = t;

191

If this changes whilst our text input has focus, we'll cache the wrong m_trackedClientOriginalMaxState

262

I would expect these two lines need swapping, otherwise you're restoring maximise before we're adjusted the client area.

mart added a comment.Mar 6 2019, 9:20 AM

I'm not super convinced about using maximise:
What if I'm typing a password in the network manager prompt in the plasma applet? or krunner? Does it look weird?

hmm, those windows would need a flag saying it's un-maximizable?
btw, the first version of this patch just resized/moved windows, was moved to maximize from the review:
https://phabricator.kde.org/D18818#409260
as Martin says it should be a bit "safer" against loop resizes. Indeed the logic now is much simpler

hmm, those windows would need a flag saying it's un-maximizable?

I don't know. Lets test it and get some screenshots of what happens.

From a spec POV we're allowed to resize a client above it's max_size, whether that works in practice is a different matter.

mart added a comment.Mar 8 2019, 10:14 AM

in the end, the fixed windows as krunner and the plasma popups, get moved to the top left corner, but not resized, which is in some sense even wronger

mart updated this revision to Diff 53425.Mar 8 2019, 10:42 AM
mart marked an inline comment as done.
  • adress a couple of comments
mart added inline comments.Mar 8 2019, 10:48 AM
shell_client.cpp
904

this is the maximized area got at line 828, which may have been adjusted to consider the keyboard space, otherwise we always get the full maximized area

virtualkeyboard.cpp
156

t is not an abstractclient but a surface?

191

hmm, that should happen only when inputPanelHasBeenOpened?
(that means, the keyboard has been set yo visible only on this updateinputpanelstate has been called)

mart added a comment.Mar 8 2019, 11:32 AM

One thing i've been thinking, what about coming up with a whole new state property for AbstractClient?
setting a client to virtualKeyboardResize (or whatever is called) would just resize it with minimal intervention, and have its own geometryRestore.
any manual resize (or maximize/unmaximize) bu the user would unset this state

not sure if this would cause bigger or smaller complexity

davidedmundson added inline comments.Mar 8 2019, 11:35 AM
virtualkeyboard.cpp
156

I meant:

m_trackedClient = waylandServer()->findAbstractClient(t);

262

This comment is done

Writing that explicitly as phab make it look like we should swap them again!

mart updated this revision to Diff 53428.Mar 8 2019, 11:46 AM
mart marked 2 inline comments as done.
  • don't recompute the same surface twice
mart marked 3 inline comments as done.Mar 8 2019, 11:46 AM
mart updated this revision to Diff 53429.Mar 8 2019, 11:55 AM
  • crash--
mart added a comment.Mar 8 2019, 11:58 AM

now, a problem which i don't see a solution for

an app that has the following QML code:
import QtQuick 2.0
import QtQuick.Controls 2.3 as Controls

ListView {

id: root
cacheBuffer: 0
width: 300; height: 400
model: 200
delegate: Controls.TextField {
    width: root.width
}

}

resize the window to take the full screen height, then click on a textfield near the bottom
the keyboard will open, the window resized, the focused text item will be out of view, destroyed and the keyboard closes immediately

(now i am fine if this is just a thing we say is an oincorrect thing for the app to do)

mart updated this revision to Diff 54281.Mar 18 2019, 10:38 PM
  • Merge branch 'master' into arcpatch-D18818
  • don't maximize
mart added a comment.Mar 18 2019, 10:39 PM

maximize didn't work very well.
now it's resizing again, but using a different saved geometry.

a timer is used to avoid loops of windows infinitely resizing

mart updated this revision to Diff 54312.Mar 19 2019, 12:52 PM
  • always try to stay as true as possible to original size

Cool. Seems to work nicely, even with any weird edge cases I've thrown at it.
In general ++

You know someone is going to say "blah blah unit test blah".
It should be easy enough if we ignore the whole virtual keyboard part and instead invoke AbstractClient::setVirtualKeyboardRect directly. I can help if needed.

Also a few more comments would be nice (i.e why there is a floodTimer)

abstract_client.cpp
79

+ clientFullscreen changed?

abstract_client.h
706

Please add a comment to say geo is relative to the global space, not the client

virtualkeyboard.cpp
152

If we move from one subsurface to another we would get into this path with the same client.

It might be worth doing

auto newClient = waylandServer()->findAbstractClient(waylandServer()->seat()->focusedTextInputSurface());
 if (newClient != m_trackedClient) {
     m_trackedClient->setVirtualKeyboardGeometry(QRect());
     m_trackedClient = newClient;
 }
mart updated this revision to Diff 54327.Mar 19 2019, 3:20 PM
mart marked 2 inline comments as done.
  • take care of subsurfaces focus change
mart updated this revision to Diff 54374.Mar 19 2019, 9:39 PM
  • add autotest
davidedmundson accepted this revision.Mar 20 2019, 9:18 AM
davidedmundson added inline comments.
autotests/integration/move_resize_window_test.cpp
982 ↗(On Diff #54374)

This line is a bit misleading, there isn't a new size requested.

You're resizing to a configure request from ages ago.

Arguably it's a nice test to show that the window hasn't moved afterwards, so maybe leave the code and change this comment.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 20 2019, 10:05 AM
This revision was automatically updated to reflect the committed changes.