Test that text input is entered after child window is closed #216

wmww · 2021-11-12T02:08:52Z

This is supposed to test canonical/mir#2189, but it passes for some reason. A problem for next week.

wmww · 2021-11-15T23:49:09Z

Ah! I forgot to actually set the parent on the child. Fixed. (and by fixed, I mean the test is now failing as it should)

wmww · 2021-11-16T02:29:41Z

canonical/mir#2223 fixes this test on Mir.

2244: Give keyboard focus to menus (grabbing popups) r=Saviq a=wmww According to XDG Shell, grabbing popups should be given keyboard focus and non-grabbing ones should not. This PR brings our keyboard focus logic in line with that. Fixes #2241. Tested by the recently-updated canonical/wlcs#216. Co-authored-by: William Wold <wm@wmww.sh> Co-authored-by: Alan Griffiths <alan@octopull.co.uk>

* Release 2.6.0 & changelog * gbm-kms: Be more relaxed in the rendering probe. (#2243) The libmali EGL implementation requires that the `gbm_device` passed in to `eglGetPlatformDisplay` be created from a DRM master fd. We can't guarantee this in the rendering probe, as the display platform may have already claimed DRM master. Instead, assume that an EGL platform which claims to support the GBM platform will be at least minimally usable with GBM. (cherry picked from commit 316b40d) * Merge #2244 2244: Give keyboard focus to menus (grabbing popups) r=Saviq a=wmww According to XDG Shell, grabbing popups should be given keyboard focus and non-grabbing ones should not. This PR brings our keyboard focus logic in line with that. Fixes #2241. Tested by the recently-updated canonical/wlcs#216. Co-authored-by: William Wold <wm@wmww.sh> Co-authored-by: Alan Griffiths <alan@octopull.co.uk> Co-authored-by: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com> Co-authored-by: William Wold <wm@wmww.sh>

RAOF

Oops! Looks like I left a review pending 😬

One non-blocking stylistic thing wrt operator wl_surface*(), one question about what we're testing that might require changes.

RAOF · 2022-02-25T06:15:30Z

tests/text_input_v3_with_input_method_v2.cpp

+TEST_F(TextInputV3WithInputMethodV2Test, text_input_does_not_enter_non_grabbing_popup)
+{
+    auto parent_surface = std::make_unique<wlcs::Surface>(app_client);
+    EXPECT_CALL(text_input, enter(parent_surface->operator wl_surface*()));


Is there a reason this isn't EXPECT_CALL(text_input, enter(static_cast<wl_surface*>(*parent_surface)))? That would seem more natural to me.

(Presumably the static_cast is necessary? Will *parent_surface implicitly-coerce into wl_surface* via operator wl_surface*()?)

I do believe some sort of explicit conversion is necessary. I, personally, like the explicit call to operator. Is there any reason to prefer static cast?

I, personally, like the explicit call to operator.

I, personally, find it strange to invoke operators by calling the operator function by name. The point of such functions is that they can be invoked using operator syntax.

If the intended use is to call the function by name then there are better names.

I'd be tempted to introduce a helper function:

auto as_wl_surface(auto const& s) -> wl_surface* { return s; }

Suggested change

EXPECT_CALL(text_input, enter(parent_surface->operator wl_surface*()));

EXPECT_CALL(text_input, enter(as_wl_surface((*parent_surface)));

Is the latest update satisfactory?

RAOF · 2022-02-25T06:20:14Z

tests/text_input_v3_with_input_method_v2.cpp

+    EXPECT_CALL(text_input, leave(parent_surface->operator wl_surface*())).Times(0);
+    EXPECT_CALL(text_input, enter(child_surface->operator wl_surface*())).Times(0);


I think these should EXPECT_CALL leave(_) and enter(_)? We want to assert that the non-grabbing popup doesn't change the test_input state, right?

… this passes)

…r leave any surface

wmww · 2023-05-08T17:13:16Z

Ready for review

RAOF

A couple of nits, but this can land.

Feel free to fix them up, or not.

(I think the workflow for this now is just to approve and then let Sophie hit the “merge” button?)

RAOF · 2023-05-15T07:45:46Z

tests/text_input_v3_with_input_method_v2.cpp

+    EXPECT_CALL(text_input, leave(parent_surface->wl_surface()));
+    EXPECT_CALL(text_input, enter(child_surface->wl_surface()));


Non-blocking, but this ordering is a protocol requirement, so we should probably enforce it.

RAOF · 2023-05-15T07:46:15Z

tests/text_input_v3_with_input_method_v2.cpp

+    EXPECT_CALL(text_input, leave(_)).Times(0);
+    EXPECT_CALL(text_input, enter(_)).Times(0);
+    auto child_xdg_surface = std::make_shared<wlcs::XdgSurfaceStable>(app_client, *child_surface);
+    auto child_xdg_toplevel = std::make_shared<wlcs::XdgPopupStable>(


Nit: child_xdg_toplevel -> child_xdg_popup

Saviq · 2023-05-15T08:10:26Z

(I think the workflow for this now is just to approve and then let Sophie hit the “merge” button?)

Not before #289 :)

wmww marked this pull request as ready for review December 7, 2021 17:06

wmww mentioned this pull request Dec 7, 2021

Give keyboard focus to menus (grabbing popups) canonical/mir#2244

Merged

wmww mentioned this pull request Feb 16, 2022

Opening Kate submenus causes menus to disappear canonical/mir#2324

Closed

RAOF requested changes Mar 18, 2022

View reviewed changes

Saviq assigned wmww May 2, 2023

wmww and others added 8 commits May 8, 2023 09:12

Test that text input is entered after child window is closed (idk why…

40871a7

… this passes)

Actually set parent on child surface

89773b9

Add XdgPositionerStable::setup_default()

0285681

Add grabbing and non-grabbing popup text input tests

edd1ad3

Make sure we generate a serial before popup grabbing

fa2f508

text_input_does_not_enter_non_grabbing_popup: expect does not enter o…

b4d0737

…r leave any surface

Add and use Surface::wl_surface() helper

08eac87

Don't use ZXDG enums for XDG shell stable

605bba9

wmww force-pushed the text-input-child-window-closed branch from 426d84b to 605bba9 Compare May 8, 2023 17:00

RAOF approved these changes May 15, 2023

View reviewed changes

Fix nits

4cdd36b

wmww added this pull request to the merge queue May 15, 2023

Merged via the queue into main with commit 40d4fe8 May 15, 2023
12 checks passed

wmww deleted the text-input-child-window-closed branch May 15, 2023 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test that text input is entered after child window is closed #216

Test that text input is entered after child window is closed #216

wmww commented Nov 12, 2021

wmww commented Nov 15, 2021

wmww commented Nov 16, 2021

RAOF left a comment

RAOF Feb 25, 2022

RAOF Feb 25, 2022

wmww Mar 18, 2022

AlanGriffiths Nov 15, 2022 •

edited

wmww Nov 15, 2022

RAOF Feb 25, 2022

wmww Mar 18, 2022

wmww commented May 8, 2023

RAOF left a comment

RAOF May 15, 2023

RAOF May 15, 2023

Saviq commented May 15, 2023

	EXPECT_CALL(text_input, enter(parent_surface->operator wl_surface*()));
	EXPECT_CALL(text_input, enter(as_wl_surface((*parent_surface)));

		EXPECT_CALL(text_input, leave(parent_surface->operator wl_surface*())).Times(0);
		EXPECT_CALL(text_input, enter(child_surface->operator wl_surface*())).Times(0);

Test that text input is entered after child window is closed #216

Test that text input is entered after child window is closed #216

Conversation

wmww commented Nov 12, 2021

wmww commented Nov 15, 2021

wmww commented Nov 16, 2021

RAOF 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

AlanGriffiths Nov 15, 2022 • edited

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

wmww commented May 8, 2023

RAOF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saviq commented May 15, 2023

AlanGriffiths Nov 15, 2022 •

edited