Add global InputMethodManager to handle inputMethod surfaces across screens #248

mariogrip · 2019-12-19T01:02:02Z

This adds a global InputMethodManager to handle inputMethod across screens. This (c++ side) was made with to be comparable with the upcoming proper multiscreen support.

This is a bit hacky for now, and should be cleaned up once proper multiscreen has landed.

This is still less hacky then the existing hardcoded multiscreen/secondscreen things currently in unity8 so ¯_(ツ)_/¯

This fixes: ubports/ubuntu-touch#1096

A second issue that was discovered that the second screen does not handle touch input correctly, so for now this is hard to use, tracked here: ubports/ubuntu-touch#1324

This adds a global InputMethodManager to handle inputMethod across screens. This (c++ side) was made with to be comparable with the upcoming proper multiscreen support. This is a bit *hacky* for now, and should be cleaned up once proper multiscreen has landed. This is still less hacky then the existing hardcoded multiscreen/secondscreen things currently in unity8 so ¯\_(ツ)_/¯ This fixes: ubports/ubuntu-touch#1096

peat-psuwit · 2019-12-19T09:52:13Z

I don't understand. We have different TLWM for different screens?

mariogrip · 2019-12-19T21:22:49Z

I don't understand. We have different TLWM for different screens?

currently yes, it was changed from some time back by canonical. but this will all be cleaned up once #164 lands, the current unity8 does not really support mulitscreen as only ONE screen has a TLWM, the second screen just runs a small qml part.

mariogrip · 2019-12-19T21:23:55Z

See https://github.com/ubports/unity8/blob/xenial/src/SecondaryWindow.cpp and https://github.com/ubports/unity8/blob/xenial/src/ShellApplication.cpp and you will see it's pretty hacky with hardcoded values :P

peat-psuwit

Well, I probably don't understand the problem enough. There's no point to create the global InputMethodManager only to be used in InputMethod.qml. What you've accomplished here can be reduced to the change to OrientedShell.qml and (warning: hand-crafted patch)

--- a/qml/Components/InputMethod.qml
+++ b/qml/Components/InputMethod.qml
@@ -34,5 +34,5 @@
         surfaceWidth: root.enabled ? width : -1
         surfaceHeight: root.enabled ? height : -1
-        surface: root.surface
+        surface: root.enabled ? root.surface : null
         onLiveChanged: {

If this is going to be used in the multi-screen PR, then it's probably better to submit it as a part of multi-screen PR. However, I won't use request change on this PR because, as I said, I probably don't understand the problem enough.

mariogrip · 2019-12-20T22:59:03Z

Well, I probably don't understand the problem enough. There's no point to create the global InputMethodManager only to be used in InputMethod.qml. What you've accomplished here can be reduced to the change to OrientedShell.qml and (warning: hand-crafted patch)
--- a/qml/Components/InputMethod.qml
+++ b/qml/Components/InputMethod.qml
@@ -34,5 +34,5 @@
         surfaceWidth: root.enabled ? width : -1
         surfaceHeight: root.enabled ? height : -1
-        surface: root.surface
+        surface: root.enabled ? root.surface : null
         onLiveChanged: {
 
If this is going to be used in the multi-screen PR, then it's probably better to submit it as a part of multi-screen PR. However, I won't use request change on this PR because, as I said, I probably don't understand the problem enough.

The reason why it got backported here is that the (really big) multiscreen pr wont land until after OTA-12 since we are so close to release.

The second screen only have a small piece of qml it runs (DisabledScreenNotice.qml) that runs in own QQuickView (in our case SecondaryWindow class). and this qml code does not have any shell components at all. So it does not have access to the TLWM at all, thats why a static qml property was needed to share the surface with SecondaryWindow's QQuickView context.

peat-psuwit · 2019-12-20T23:23:12Z

I still don't understand. I don't see the change in SecondaryWindow, so I don't understand how this will make OSK appear in the secondary screen. Did you miss something?

mariogrip · 2019-12-21T01:01:52Z

I still don't understand. I don't see the change in SecondaryWindow, so I don't understand how this will make OSK appear in the secondary screen. Did you miss something?

No, everything here. and i can confirm it works :)

So without this the secondscreens qml code wont have the input surface.
So the tree of DisabledScreenNotice.qml [1] is

DisabledScreenNotice.qml ->
VirtualTouchPad.qml ->
InputMethod.qml

as you see InputMethod is required by VirtualTouchPad.qml [2], but VirtualTouchPad.qml has no way of setting the surface as it does not have any TLWM or access to Shell.qml's TLWM. So this addes a static qmlSingletonType, and the shell will then set the input surface when the inputMethod sets, TLWM will then set InputMethodManager::instance()->setWindow(window); and since thats a static type it will be shared across QQuickView context's. and thats why i changed the surface var in InputMethod.qml to

property var surface: root.enabled ? InputMethodManager.surface : null;

It gets the surface from InputMethodManager, and since its a static type, it will be available across all QQuickView's. The key point here is that DisabledScreenNotice.qml does not have access to the TLWM since it's not a static type, and thus will not be shared or accessible across QQuickView

[1] https://github.com/ubports/unity8/blob/xenial/qml/DisabledScreenNotice.qml
[2] https://github.com/ubports/unity8/blob/xenial/qml/Components/VirtualTouchPad.qml#L246

peat-psuwit · 2019-12-23T18:38:24Z

I see now. InputMethod in VirtualTouchPad, what a confusing name!

IMO maybe TLWM shouldn't bother with the input method surface at all and InputMethodManager can listen directly to SurfaceManager::instance(). However, if you said it's temporary, then maybe this works too.

Also, you said it's backported. However, I can't find where it's backported from (I've looked in the multiscreen branch/PR). Could you please point me where this is backported from?

mariogrip · 2019-12-23T18:55:29Z

I see now. InputMethod in VirtualTouchPad, what a confusing name!

IMO maybe TLWM shouldn't bother with the input method surface at all and InputMethodManager can listen directly to SurfaceManager::instance(). However, if you said it's temporary, then maybe this works too.

Also, you said it's backported. However, I can't find where it's backported from (I've looked in the multiscreen branch/PR). Could you please point me where this is backported from?

It's not backported in that sense, its something i made for the new multiscreen things and backported it here, I still have those fixes for multiscreen localy.

mariogrip · 2019-12-23T18:57:14Z

but the new multiscreen handles the TLWM a bit differently, also it handles shells differently.

mariogrip · 2019-12-23T18:58:16Z

I see now. InputMethod in VirtualTouchPad, what a confusing name!

IMO maybe TLWM shouldn't bother with the input method surface at all and InputMethodManager can listen directly to SurfaceManager::instance(). However, if you said it's temporary, then maybe this works too.

Also, you said it's backported. However, I can't find where it's backported from (I've looked in the multiscreen branch/PR). Could you please point me where this is backported from?

for now i think this is fine, i dont really want to change too much in TLWM about this as this will get a major overhaul when the multiscreen/workspaces lands.

peat-psuwit

Ok. Then I won't make any further objection to the method here. Hope to see the complete PR soon!

However, I would like to request a change to the header. I'm sorry about this, especially when there's a long delay.

Otherwise, LGTM, but as my familiarity with Unity 8 is limited, someone else (@UniversalSuperBox!) must approve.

peat-psuwit · 2019-12-23T19:11:21Z

plugins/WindowManager/InputMethodManager.h

+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once


Hmm, none of the headers in this directory are using #pragma once. For consistency, please use a normal header guard define.

Suggested change

#pragma once

#ifndef INPUTMETHODMANAGER_H

#define INPUTMETHODMANAGER_H

I disagree, we should use newer c++ when possible, using pragma once is faster, cleaner and safer. I have been using pragma once everywhere else (on other parts).

Hmm, IMHO if we're gonna use pragma, we should migrate at the repository level, and as a separated PR.

@UniversalSuperBox, could you please comment?

It's supported by both gcc (since 3.4) and clang (since somewhere around 2008), the only compilers I can see us ever being worried about. If both serve roughly the same purpose, #pragma once seems like the more clear define guard.

Wait a minute, from https://gcc.gnu.org/onlinedocs/gcc-2.95.3/cpp_1.html#SEC8

There is also an explicit directive to tell the preprocessor that it need not include a file more than once. This is called #pragma once, and was used in addition to the #ifndef conditional around the contents of the header file. #pragma once is now obsolete and should not be used at all.

(Emphasis mine)

My goodness, what a complex issue. Microsoft recommends its use, GCC and ARM do not, Intel appears to have removed it from the newer versions of their compiler, the community loves it...

Ultimately, it does not matter which is used. Please create a PR to replace this #pragma once if you'd rather change it.

The one from GCC is version 2.95 released decades ago, the reason why that was a thing is there was a bug [1] that has been fixed. New docs [2] says nothing about it being obsolete, it only says it's a less portable version and that is true but we use compiler flags that is less portable too so for us it does not matter (as it's way more portable then some flags we use). What other compiler do we expect to use that does not support pragma once? See https://en.wikipedia.org/wiki/Pragma_once#Portability

Also this sums it up pretty nicely https://www.reddit.com/r/cpp/comments/4cjjwe/come_on_guys_put_pragma_once_in_the_standard/

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=11569
[2] https://gcc.gnu.org/onlinedocs/gcc-9.2.0/cpp/Pragmas.html#Pragmas

peat-psuwit · 2019-12-23T19:13:07Z

plugins/WindowManager/InputMethodManager.h

+    unity::shell::application::MirSurfaceInterface* surface() const;
+
+     Window* m_inputMethodWindow{nullptr};
+};


Suggested change

};

};

#endif // INPUTMETHODMANAGER_H

UniversalSuperBox · 2020-01-08T20:34:55Z

Other than the tests failing because the theme changed (grumble grumble), this PR does not work correctly with the Launcher. Every time the Shell starts, the Launcher comes on screen.

Discussed in today's development sync, #pragma once should be used in the future or when we refactor things.

UniversalSuperBox

I am now unable to reproduce the Launcher appearing on boot. I get the input method on the correct screen in multi-display, though!

UniversalSuperBox · 2020-01-09T02:45:26Z

Merging with unsuccessful checks, the pr-merge task is, ironically, not merging with the most up-to-date target branch.

mariogrip requested review from peat-psuwit and UniversalSuperBox December 19, 2019 01:02

peat-psuwit reviewed Dec 20, 2019

View reviewed changes

peat-psuwit previously requested changes Dec 23, 2019

View reviewed changes

UniversalSuperBox approved these changes Jan 9, 2020

View reviewed changes

UniversalSuperBox merged commit 1b30ec8 into xenial Jan 9, 2020

UniversalSuperBox deleted the xenial_-_secondscreenosk branch January 9, 2020 02:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add global InputMethodManager to handle inputMethod surfaces across screens #248

Add global InputMethodManager to handle inputMethod surfaces across screens #248

mariogrip commented Dec 19, 2019

peat-psuwit commented Dec 19, 2019

mariogrip commented Dec 19, 2019

mariogrip commented Dec 19, 2019

peat-psuwit left a comment

mariogrip commented Dec 20, 2019

peat-psuwit commented Dec 20, 2019

mariogrip commented Dec 21, 2019 •

edited

peat-psuwit commented Dec 23, 2019

mariogrip commented Dec 23, 2019

mariogrip commented Dec 23, 2019

mariogrip commented Dec 23, 2019

peat-psuwit left a comment

peat-psuwit Dec 23, 2019

mariogrip Jan 2, 2020

peat-psuwit Jan 5, 2020

UniversalSuperBox Jan 8, 2020

peat-psuwit Jan 12, 2020

UniversalSuperBox Jan 13, 2020

mariogrip Jan 13, 2020

peat-psuwit Dec 23, 2019

UniversalSuperBox commented Jan 8, 2020

UniversalSuperBox left a comment

UniversalSuperBox commented Jan 9, 2020

	#pragma once
	#ifndef INPUTMETHODMANAGER_H
	#define INPUTMETHODMANAGER_H

-};
+};
+#endif // INPUTMETHODMANAGER_H

Add global InputMethodManager to handle inputMethod surfaces across screens #248

Add global InputMethodManager to handle inputMethod surfaces across screens #248

Conversation

mariogrip commented Dec 19, 2019

peat-psuwit commented Dec 19, 2019

mariogrip commented Dec 19, 2019

mariogrip commented Dec 19, 2019

peat-psuwit left a comment

Choose a reason for hiding this comment

mariogrip commented Dec 20, 2019

peat-psuwit commented Dec 20, 2019

mariogrip commented Dec 21, 2019 • edited

peat-psuwit commented Dec 23, 2019

mariogrip commented Dec 23, 2019

mariogrip commented Dec 23, 2019

mariogrip commented Dec 23, 2019

peat-psuwit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UniversalSuperBox commented Jan 8, 2020

UniversalSuperBox left a comment

Choose a reason for hiding this comment

UniversalSuperBox commented Jan 9, 2020

mariogrip commented Dec 21, 2019 •

edited