New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] On-screen keyboard support #892

Closed
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@dcz-purism
Contributor

dcz-purism commented Apr 23, 2018

This pull request combines work on:

  • input-method protocol #856
  • text-input protocol #776
  • rootston glue

Its purpose is to develop these in sync, and to remove the need to juggle different long-running feature branches as different parts are being developed. This has proven to take up effort and lead to awkward situations, e.g. many of the rebased rootston commits were developed against an earlier version of wlr_input_method.h and are currently unbuildable.

One interesting thing is the addition of code duplicating some functionality of layer surfaces (panels, unfocusable windows) in rootston views, in order to support input-method-panel.

panel->panel = wl_global_create(display, &zwp_input_panel_v1_interface, 1,
panel, input_panel_bind);
return panel;
}

This comment has been minimized.

@ddevault

ddevault Apr 23, 2018

Member

This should not be named osk.c, input-method supports more than OSKs

@agx

This comment has been minimized.

Contributor

agx commented Apr 23, 2018

Now that we have layer-surfaces can't OSK use just that?

@dcz-purism

This comment has been minimized.

Contributor

dcz-purism commented Apr 23, 2018

@agx: I wanted to have a reasonably complete implementation of the existing protocol first, especially because all existing software, namely weston-keyboard uses it. The plan is to update the protocol and remove the panel in its current form in favor of layer-surfaces in the future.

@agx

This comment has been minimized.

Contributor

agx commented Apr 23, 2018

@dcz-purism with existing you mean weston-keyboard using zwp_input_panel_surface_v1 ?
Looking at the protocol the panel part looks pretty much bolted onto generic input_method stuff.

@ddevault

I'll come back around for another round of review when input-method is disentangled from rootston views

struct roots_input_panel *roots_input_panel_create(struct wl_display *display) {
struct roots_input_panel *panel = calloc(1,
sizeof(struct roots_input_panel));

This comment has been minimized.

@ddevault

ddevault Apr 23, 2018

Member

Bad indent

#include "input-method-unstable-v1-protocol.h"
static void noop() {}

This comment has been minimized.

@ddevault

ddevault Apr 23, 2018

Member

Add newlines and such and something to the effect of /* This space deliberately left blank */

#include "rootston/desktop.h"
struct roots_input_panel {
struct roots_desktop *desktop; // FIXME - remove when views come

This comment has been minimized.

@ddevault

ddevault Apr 23, 2018

Member

If you expect this TODO to get merged, can you elaborate a bit? If you will resolve prior to merge no worries

struct wlr_text_input *input;
struct wlr_input_method_context *context;
struct wl_list link;

This comment has been minimized.

@ddevault

ddevault Apr 23, 2018

Member

Wrong indent here and sprinkled throughout this struct

struct wlr_input_method_context *context;
struct wl_list link;
struct wl_listener text_input_enable; // uint32_t show_input_panel

This comment has been minimized.

@ddevault

ddevault Apr 23, 2018

Member

Put the docs near the signal, not the listener

struct wl_resource *resource;
struct {
struct wl_signal new_context; // struct wlr_input_method_context*
// TODO: create roots_keyboard (seat_add_keyboard) on this; modify roots_keyboard to support backend-less

This comment has been minimized.

@ddevault

ddevault Apr 23, 2018

Member

I think this should be more along the lines of wlr_input_method creating a wlr_keyboard which roots_keyboard should just be able to pick up without changes. Maybe some silly changes to automatically associate it with the relevant seat if wlr_keyboard_is_input_method returns true.

This comment has been minimized.

@dcz-purism

dcz-purism Apr 23, 2018

Contributor

I think this is what I did but forgot to update the comment. Thanks for the seat input.

void wlr_input_method_send_deactivate(struct wlr_input_method *input_method,
struct wlr_input_method_context *context);
#endif

This comment has been minimized.

@ddevault

ddevault Apr 23, 2018

Member

minor nit: extra newline above the #endif

roots_seat_set_focus(seat, view);
if (!view || !view->special || view->features.focusable) {
roots_seat_set_focus(seat, view);
}

This comment has been minimized.

@ddevault

ddevault Apr 23, 2018

Member

Weird indent

view->x = x;
view->y = y;
view_damage_whole(view);
}

This comment has been minimized.

@ddevault

ddevault Apr 23, 2018

Member

Yeah all of this stuff can go if you don't try to hack it into views.

This comment has been minimized.

@dcz-purism

dcz-purism Apr 23, 2018

Contributor

I'll be slow to address it, but it will get addressed eventually. I want to keep the whole stack working at any time, so I'll get rid of the current panel implementation once there's a keyboard that doesn't use it. It probably means implementing it on my own, so it will take time.

@@ -43,7 +43,7 @@ struct roots_seat *input_get_seat(struct roots_input *input, char *name) {
return seat;
}
static void handle_new_input(struct wl_listener *listener, void *data) {
void handle_new_input(struct wl_listener *listener, void *data) {

This comment has been minimized.

@ddevault
@dcz-purism

This comment has been minimized.

Contributor

dcz-purism commented Apr 23, 2018

@agx: yes, it's using input-method-panel-surface, which is definitely bolted, but it covers some things that layer-shell doesn't:

<SirCmpwn> input-method popups are shown by the compositor near the cursor of the client with keyboard focus
<SirCmpwn> xdg-shell and layer-shell popups are shown where the client asks them to be

and also the function of keyboards that they may need to be visible even when the lock screen is active.

@Shugyousha

Mostly superficial things

@@ -333,6 +336,9 @@ static bool has_standalone_surface(struct roots_view *view) {
case ROOTS_XWAYLAND_VIEW:
return wl_list_empty(&view->xwayland_surface->children);
#endif
case ROOTS_INPUT_PANEL_VIEW:
printf("Stub: has_standalone_surface on OSK_VIEV returns true\n");
return true;

This comment has been minimized.

@Shugyousha

Shugyousha Apr 23, 2018

Contributor

The printed text should be updated to correspond to the constant. There is also a typo in the current text (s/OSK_VIEV/OSK_VIEW/).

This comment has been minimized.

@dcz-purism

dcz-purism Apr 24, 2018

Contributor

What's a standalone surface here?

This comment has been minimized.

@emersion

emersion Apr 24, 2018

Member

It's used for fullscreen to know if the view has only one surface.

first_seat_view->link.next, next_seat_view, link);
// Focus the next view which is focusable
struct roots_seat_view *next_seat_view = first_seat_view;;
do {

This comment has been minimized.

@Shugyousha

Shugyousha Apr 23, 2018

Contributor

double ;

}
if (text_input->pending.surrounding.text) {
free(text_input->pending.surrounding.text);
}

This comment has been minimized.

@Shugyousha

Shugyousha Apr 23, 2018

Contributor

Since free(NULL) is a noop, we don't need these two "if" statements here.

if (text_input->pending.surrounding.text) {
free(text_input->pending.surrounding.text);
}
text_input->pending.surrounding.text = strdup(text);

This comment has been minimized.

@Shugyousha

Shugyousha Apr 23, 2018

Contributor

Same here

if (text_input->current.surrounding.text) {
free(text_input->pending.surrounding.text);
}
text_input->current = text_input->pending;

This comment has been minimized.

@Shugyousha

Shugyousha Apr 23, 2018

Contributor

Ditto

struct wlr_text_input_manager *wlr_text_input_manager_create(
struct wl_display *wl_display) {
struct wlr_text_input_manager *manager =
calloc(1, sizeof(struct wlr_text_input_manager));

This comment has been minimized.

@Shugyousha

Shugyousha Apr 23, 2018

Contributor

Add out of memory check

@agx

This comment has been minimized.

Contributor

agx commented Apr 24, 2018

@dcz-purism my main concern is growing a third way to handle popups and overlay surfaces. layer-shell already has overlap with views and it would be nice if either one could be reused.

dcz-purism added some commits Apr 11, 2018

rootston: Support on-screen keyboards
On-screen keyboard will appear whenever an application using the text-input protocol wants to edit text. Rootston will pass text composition events from the keyboard to the application, and will accept keysym events from the keyboard, and create a virtual keyboard device in order to pass them to the application. The keyboard will restrict the space available for maximized applications.

rootston/text_input is the relay of messages between wlr_text_input and wlr_input_method.

To test with older GTK installed:

```
cd gtk
PKG_CONFIG_PATH=.../wayland_protocols_prefix/share/pkgconfig ./configure --enable-wayland-backend --enable-x11-backend --prefix=.../gtk-install
make && make install
chrpath -d .../gtk-install/lib/gtk-3.0/3.0.0/immodules/im-wayland.so
WAYLAND_DEBUG=1 GDK_BACKEND=wayland GTK_IM_MODULE=wayland GTK_IM_MODULE_FILE=.../gtk-install/lib/gtk-3.0/3.0.0/immodules.cache yad --entry
```
/**
* Usage: idle-inhibit
* Creates a xdg-toplevel using the idle-inhibit protocol.

This comment has been minimized.

@agx

agx Apr 26, 2018

Contributor

It's rather text-input and does s.th. different?

This comment has been minimized.

@dcz-purism

dcz-purism Apr 26, 2018

Contributor

Thanks. I didn't realize this push would send out notifications.

*im_pid = fork();
if (*im_pid == 0) {
sleep(1);
execl("/bin/sh", "/bin/sh", "-c", im_cmd, (void *)NULL);

This comment has been minimized.

@agx

agx Apr 26, 2018

Contributor

Why does it need a shell?

This comment has been minimized.

@dcz-purism

dcz-purism Apr 26, 2018

Contributor

The -E option pipes the command through a shell, so in order to maintain consistency, I made it the same. Personally, I don't see the need for a shell in either case, but maybe I'm missing something.

@agx

This comment has been minimized.

Contributor

agx commented Apr 27, 2018

@agx agx changed the title from WIP: On-screen keyboard support to [WIP] On-screen keyboard support Apr 27, 2018

bool enabled; // client requested state
bool sent_enter; // server requested state
bool active; // active focus

This comment has been minimized.

@agx

agx Apr 27, 2018

Contributor

nitpick: Indentation is off here

@ddevault

This comment has been minimized.

Member

ddevault commented Jul 29, 2018

How're are things in this department going on your end? Should we expect an update to this soon?

@dcz-purism

This comment has been minimized.

Contributor

dcz-purism commented Jul 29, 2018

Yes indeed! Currently I'm busy upstreaming text-input to GTK, so after the merge window closes and I'm free to do other things again, I'll push my changes to get some formal feedback.

@ddevault

This comment has been minimized.

Member

ddevault commented Jul 29, 2018

Sweet, keep up the good work!

@dcz-purism

This comment has been minimized.

Contributor

dcz-purism commented Aug 6, 2018

The first replacement of the old input method was just posted to gather feedback today:

https://lists.freedesktop.org/archives/wayland-devel/2018-August/039255.html

@dcz-purism

This comment has been minimized.

Contributor

dcz-purism commented Aug 29, 2018

I rewrote this code to updated protocols and submitted it as a separate (out of forgetfulness) PR here.

@dcz-purism dcz-purism closed this Aug 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment