New keyboard interactivity options #105

dkondor · 2021-01-07T02:38:35Z

Support more settings for keyboard interactivity, according to the changes suggested to the protocol here: swaywm/wlr-protocols#100

By opening this pull request, I agree for my modifications to be licensed under whatever licenses are indicated at the start of the files I modified

New setting allows "normal" keyboard focus semantics for windows on any layer. Proposed for new version of the protocol.

wmww

A few nits, but the idea looks good. I only reviewed the library, not the examples. Of course merging is blocked on the upstream PR getting merged, so no rush to fix these things.

wmww · 2021-01-07T10:15:51Z

include/gtk-layer-shell.h

 */
-void gtk_layer_set_keyboard_interactivity (GtkWindow *window, gboolean interacitvity);
+void gtk_layer_set_keyboard_interactivity (GtkWindow *window, GtkLayerShellKeyboardInteractivity interactivity);


This and the below function break backwards compatibility. We'll want to deprecate the old functions but keep them around.

Since true == 1 and false == 0, this breaks ABI but doesn't break API. Depending on the project policy, this may or may not be okay.

I am trying to keep the ABI stable (various versions are shipped with various distros, and apps should ideally work with all of them). It might even be ABI-compatible (enum types seem to be implementation defined), but no reason to play with fire here. Renaming the new function gtk_layer_set_keyboard_interactivity_type (or something) and keeping the old one marked as deprecated is not a great burden.

Makes sense.

Sure, I've added new functions that are called by these.

src/api.c

wmww · 2021-01-07T10:26:38Z

src/api.c

+        g_warning ("Invalid value for keyboard interactivity setting!\n");
+        return;
+    }
+    if (version <= 3 && interactivity == GTK_LAYER_SHELL_KEYBOARD_ON_DEMAND)


Might be a little cleaner to make this check in layer-surface.c. I think getting the version from the zwlr_layer_surface_v1 (which you'll have access to in that file) instead of from the layer shell global would be slightly preferable.

zwlr_layer_surface_v1_get_version can be used for this purpose. EDIT: ah, you're already using a similar function, cool. Some people aren't aware that these exist.

Moved the check there. The problem is that self->layer_surface can be NULL when this is called, so in that case, we still have to use the global object as fallback (I think it is cleaner to have the warning here and not have to make this check when committing the surface later).

src/api.c

src/layer-surface.c

src/layer-surface.h

emersion · 2021-01-08T10:25:19Z

examples/demo/gtk-layer-demo.c

+        default_keyboard_interactivity = GTK_LAYER_SHELL_KEYBOARD_NONE;
+    } else if (value[0] == '1' && value[1] == 0) {
+        default_keyboard_interactivity = GTK_LAYER_SHELL_KEYBOARD_EXCLUSIVE;
+    } else if (value[0] == '2' && value[1] == 0) {


0/1 could be handled to retain old behavior, but I wonder if it makes sense to handle 2. Using enum names is clearer.

Handling numbers seems unnecisarry. Just handling names should be fine. (I still haven't reviewed the changes to the examples fully)

emersion · 2021-01-12T10:23:54Z

@wmww Ack on merging the wlr-protocols PR?

wmww · 2021-01-12T19:40:22Z

Now that the upstream PR has merged I'll give this a full review when I have time. These don't block my initial review, but they will need to be done before merging:

Pull in the final version of the layer shell protocol
Remove the new example (we don't need individual examples for every feature, adding interactivity support to the demo is enough)
Add integration tests (see test-set-keyboard-interactivity.c, test-get-keyboard-interactivity.c for examples, and test/README.md for full test documentation (though you shouldn't need to understand most of it))

P.S. let me know if anything about writing tests could be documented better.

dkondor · 2021-01-16T14:15:39Z

Now that the upstream PR has merged I'll give this a full review when I have time. These don't block my initial review, but they will need to be done before merging:
* Pull in the final version of the layer shell protocol

Done already :)

* Remove the new example (we don't need individual examples for every feature, adding interactivity support to the demo is enough)

I'm not sure what you mean; the new files are part of the demo, just separated for clarity (I can merge them with one of the other ones).

* Add integration tests (see [test-set-keyboard-interactivity.c](https://github.com/wmww/gtk-layer-shell/blob/master/test/integration-tests/test-set-keyboard-interactivity.c), [test-get-keyboard-interactivity.c](https://github.com/wmww/gtk-layer-shell/blob/master/test/integration-tests/test-get-keyboard-interactivity.c) for examples, and [test/README.md](https://github.com/wmww/gtk-layer-shell/blob/master/test/README.md) for full test documentation (though you shouldn't need to understand most of it))

I did a first try at this by adding the following two files:
test/integration-tests/test-get-keyboard-interactivity-types.c
test/integration-tests/test-set-keyboard-interactivity-types.c

To make this work, I had to update the mock server to advertise version 4 of the protocol. I'm wondering if there should be an extra test that gtk-layer-shell behaves as expected on a lower server version and if that's easily achievable with the current setup.

dkondor · 2021-01-16T14:16:21Z

Also, I'm marking this ready for review since the protocol and wlroots PRs have merged and the main functionality is implemented.

wmww · 2021-01-17T01:47:03Z

test/integration-tests/test-get-keyboard-interactivity-types.c

+    gtk_layer_set_keyboard_interactivity_type(window, GTK_LAYER_SHELL_KEYBOARD_ON_DEMAND);
+    ASSERT_EQ(gtk_layer_get_keyboard_interactivity_type(window), version >= 4 ? GTK_LAYER_SHELL_KEYBOARD_ON_DEMAND : GTK_LAYER_SHELL_KEYBOARD_NONE, "%d");
+    gtk_widget_show_all(GTK_WIDGET(window));
+    ASSERT_EQ(gtk_layer_get_keyboard_interactivity_type(window), version >= 4 ? GTK_LAYER_SHELL_KEYBOARD_ON_DEMAND : GTK_LAYER_SHELL_KEYBOARD_NONE, "%d");


Since this will always be run against the mock server, no need for the version checks. We can assume it's on the expected version.

EDIT: I did that

To answer your question, yes, ideally we'd have additional tests for different protocols versions but we're not set up to easily implement that right now, so don't bother. I'll add those tests if/when I set up a way to make the mock server use different versions of protocols.

wmww · 2021-01-17T01:55:08Z

I'm not sure what you mean...

Either the thing I was talking about was removed or I was just confused. Either way it's fine now.

…tions in errors

…ctions

…version()

…eyboard_mode()

…ely being used incorrectly

wmww · 2021-01-17T03:32:56Z

All right as you can see I've added a bunch of changes, including notably changing the names from "keyboard_interactivity_type" to "keyboard_mode". That diverges from the protocol terminology, but I think it's a little clearer, much shorter and less likely to be confused with the deprecated functions. Much appreciated if you want to give the PR a review in it's current state. It seems ready to merge to me.

Rename keyboard interactivity enum

dkondor · 2021-01-17T04:40:47Z

All seems good, I've only added a minor edit in one place where the documentation was inconsistent.

Otherwise, I've just tested a bit and seems to work well.

wmww · 2021-01-17T20:48:08Z

Awesome! Thanks!

dkondor added 2 commits January 7, 2021 10:30

Allow new values for keyboard interactivity

97a4cd0

New setting allows "normal" keyboard focus semantics for windows on any layer. Proposed for new version of the protocol.

Update demo with new keyboard interactivity options

e58d2fa

dkondor mentioned this pull request Jan 7, 2021

Proposal for more options regarding keyboard interactivity for layer-shell clients swaywm/wlr-protocols#100

Merged

wmww reviewed Jan 7, 2021

View reviewed changes

emersion reviewed Jan 8, 2021

View reviewed changes

Address review (library)

9ebafd2

dkondor force-pushed the new_keyboard_interactivity branch from aa1b9b9 to 9ebafd2 Compare January 9, 2021 04:35

dkondor added 2 commits January 9, 2021 12:48

Use enum in LayerSurface

89ce206

Fix crash if LayerSurface does not have a Wayland object yet

7431c50

dkondor added 4 commits January 16, 2021 21:56

test for new keyboard interactivity types

22972df

demo: only accept string options for keyboard interactivity

6c51b8c

add test for setting keyboard interactivity

27ae389

mock-server: set layer-shell version to 4

296fcce

dkondor marked this pull request as ready for review January 16, 2021 14:16

wmww reviewed Jan 17, 2021

View reviewed changes

wmww added 10 commits January 16, 2021 18:01

Use compatible enum types to fix warning

629ea16

Merge branch 'master' into new_keyboard_interactivity

6b8751e

Update keyboard demo option description

df7eeea

Keyboard demo option on_demand -> on-demand

c793d0b

Standardize on single quotes in option descriptions and show valid op…

1caa8b0

…tions in errors

Keyboard selector demo tooltip

85cb83c

Add correct versions for new functions and deprecations

bb36aab

Restore old docs for deprecated functions and move them below new fun…

ec7884d

…ctions

Document that enum *_ENTRY_NUMBER may change in future releases

82667fd

Rename gtk_layer_get_layer_shell_version() -> gtk_layer_get_protocol_…

2963dc3

…version()

wmww added 3 commits January 16, 2021 19:13

Rename gtk_layer_set_keyboard_interactivity_type() -> gtk_layer_set_k…

9cd93e9

…eyboard_mode()

Add warnings when deprecated keyboard interactivity functions are lik…

63f29e6

…ely being used incorrectly

Fix docs

a3f0713

Small fix for documentation

8e0c185

Rename keyboard interactivity enum

wmww merged commit f7aa237 into wmww:master Jan 17, 2021

dkondor deleted the new_keyboard_interactivity branch January 18, 2021 01:42

This was referenced Jul 23, 2023

zwlr_layer_surface_v1::keyboard_interactivity v4 swaywm/sway#7499

Open

Add keyboard mode flag to support both: "on-demand" & "exclusive" GTK layer-shell modes nwg-piotr/nwg-drawer#95

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New keyboard interactivity options #105

New keyboard interactivity options #105

dkondor commented Jan 7, 2021

wmww left a comment

wmww Jan 7, 2021

emersion Jan 8, 2021 •

edited

Loading

wmww Jan 8, 2021

emersion Jan 8, 2021

dkondor Jan 9, 2021

wmww Jan 7, 2021

emersion Jan 8, 2021 •

edited

Loading

dkondor Jan 9, 2021

emersion Jan 8, 2021

wmww Jan 8, 2021 •

edited

Loading

emersion commented Jan 12, 2021

wmww commented Jan 12, 2021

dkondor commented Jan 16, 2021

dkondor commented Jan 16, 2021

wmww Jan 17, 2021 •

edited

Loading

wmww commented Jan 17, 2021

wmww commented Jan 17, 2021 •

edited

Loading

dkondor commented Jan 17, 2021

wmww commented Jan 17, 2021

New keyboard interactivity options #105

New keyboard interactivity options #105

Conversation

dkondor commented Jan 7, 2021

wmww left a comment

Choose a reason for hiding this comment

wmww Jan 7, 2021

Choose a reason for hiding this comment

emersion Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

wmww Jan 8, 2021

Choose a reason for hiding this comment

emersion Jan 8, 2021

Choose a reason for hiding this comment

dkondor Jan 9, 2021

Choose a reason for hiding this comment

wmww Jan 7, 2021

Choose a reason for hiding this comment

emersion Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

dkondor Jan 9, 2021

Choose a reason for hiding this comment

emersion Jan 8, 2021

Choose a reason for hiding this comment

wmww Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

emersion commented Jan 12, 2021

wmww commented Jan 12, 2021

dkondor commented Jan 16, 2021

dkondor commented Jan 16, 2021

wmww Jan 17, 2021 • edited Loading

Choose a reason for hiding this comment

wmww commented Jan 17, 2021

wmww commented Jan 17, 2021 • edited Loading

dkondor commented Jan 17, 2021

wmww commented Jan 17, 2021

emersion Jan 8, 2021 •

edited

Loading

emersion Jan 8, 2021 •

edited

Loading

wmww Jan 8, 2021 •

edited

Loading

wmww Jan 17, 2021 •

edited

Loading

wmww commented Jan 17, 2021 •

edited

Loading