Skip to content
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

Support input method and text input #1203

Merged
merged 4 commits into from Oct 12, 2018

Conversation

Projects
None yet
6 participants
@dcz-purism
Copy link
Contributor

dcz-purism commented Aug 29, 2018

This change introduces basic but useful support for zwp-text-input-v3 and zwp-input-method-v2 protocols.

The compositor becomes a relay between the input method program and the user-facing text input application, passing messages between them, and gracefully handling connections and disconnections.

The text-input protocol is implemented fully. The basic and least controversial pieces to make input-method functionality complete are implemented as well. Remaining are the grab and popup functionality. What's currently implemented is enough to support virtual keyboards, but probably not enough for e.g. CJK input methods.

The structure doing most of the work is the rootston/text_input.h::roots_input_method_relay. One is attached to each seat, and each manages multiple text-input connections and zero or one input-method connections.

Testing possible using virtboard's input_method branch (at scaling factor x1) and gtk-3.24.

To test with system-wide GTK:

$WLROOTS/rootston/rootston &
$VIRTBOARD/virtboard &
yad --entry &

With locally-installed GTK, the yad line can look like this:

LD_LIBRAY_PATH=$GTK/lib yad --entry

or, in the unlikely case of issues:

GDK_BACKEND=wayland GTK_IM_MODULE=wayland GTK_IM_MODULE_FILE=$GTK/lib/gtk-3.0/3.0.0/immodules.cache yad --entry
@dcz-purism

This comment has been minimized.

Copy link
Contributor Author

dcz-purism commented Aug 31, 2018

Script for easier testing:

#!/bin/bash
set -e
PREFIX=`pwd`/local
BASE=`pwd`
export PATH=$PREFIX/bin/:$PATH
export PKG_CONFIG_PATH=$PREFIX/share/pkgconfig/:$PREFIX/lib/pkgconfig/

sudo apt-get -y update
sudo apt-get -y install wget git yad build-essential

# wayland 1.16 not in Debian
sudo apt-get -y install libffi-dev libxml2-dev automake libexpat1-dev
wget https://wayland.freedesktop.org/releases/wayland-1.16.0.tar.xz
tar -xf wayland-1.16.0.tar.xz
cd wayland-1.16.0
aclocal
automake
./configure --disable-documentation --prefix=$PREFIX
make -j4
make install

cd $BASE

# build virtboard
sudo apt-get -y install debhelper meson pkg-config libpixman-1-dev libpng-dev libxcb1-dev libxcb-xkb-dev libxkbcommon-dev libcairo2-dev libwayland-dev wayland-protocols
git clone https://source.puri.sm/Librem5/virtboard.git -b input_method
export LC_ALL=C.UTF-8
meson virtboard virtboard_build
ninja -C virtboard_build

# build rootston
sudo apt-get -y install debhelper libcap-dev libdrm-dev libegl1-mesa-dev libgbm-dev libgles2-mesa-dev libinput-dev libpixman-1-dev libsystemd-dev libwayland-dev libxcb1-dev libxcb-composite0-dev libxcb-icccm4-dev libxcb-image0-dev libxcb-render0-dev libxcb-xfixes0-dev libxkbcommon-dev meson pkg-config wayland-protocols build-essential
git clone https://github.com/dcz-purism/wlroots.git -b input
meson wlroots wlroots_build
ninja -C wlroots_build

# build GTK
sudo apt-get -y install gtk-doc-tools gobject-introspection libatk1.0-dev libgirepository1.0-dev libepoxy-dev libpango1.0-dev libgdk-pixbuf2.0-dev
git clone --depth 1 https://gitlab.gnome.org/GNOME/gtk.git -b gtk-3-24
cd gtk
./autogen.sh --disable-x11-backend --enable-wayland-backend --disable-documentation --prefix=$PREFIX
make -j4
make install

exit 0

# run a demo - better play with all the programs yourself

cd $BASE
./wlroots_build/rootston/rootston &
./virtboard_build/virtboard &
yad &
LD_LIBRARY_PATH=$PREFIX/lib yad --entry &
@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Aug 31, 2018

Thanks. I intend to get around to testing this over the weekend.

context->pending.preedit.cursor_end = cursor_end;
free(context->pending.preedit.text);
context->pending.preedit.text = strdup(text); // TODO: clean up
}

This comment has been minimized.

Copy link
@agx

agx Aug 31, 2018

Contributor

This leaks on destroy.

struct wlr_input_method *context =
wl_resource_get_user_data(resource);
free(context->pending.commit_text);
context->pending.commit_text = strdup(text); // TODO: clean up

This comment has been minimized.

Copy link
@agx

agx Aug 31, 2018

Contributor

This leaks on destroy.

'text-input': {
'src': 'text-input.c',
'dep': [wayland_cursor, wayland_client, wlr_protos, wlroots],
},

This comment has been minimized.

Copy link
@agx

agx Aug 31, 2018

Contributor

You're adding both in the same commi alhough one should be in text-input and the other one in input-method.

This comment has been minimized.

Copy link
@dcz-purism

dcz-purism Sep 26, 2018

Author Contributor

Should be fixed now.

'screenshooter.xml',
'text-input-unstable-v3.xml',
'wlr-export-dmabuf-unstable-v1.xml',

This comment has been minimized.

Copy link
@agx

agx Aug 31, 2018

Contributor

You're adding both in the same commit although one should be in text input and the other one in input-method (to make reverts, etc simpler).

This comment has been minimized.

Copy link
@dcz-purism

dcz-purism Sep 26, 2018

Author Contributor

Should be fixed now.

@ddevault
Copy link
Member

ddevault left a comment

A lot of this still looks WIP, definitely fix up the TODOs and FIXMEs. Overall code looks good

@@ -0,0 +1,214 @@
#ifndef _POSIX_C_SOURCE
#define _POSIX_C_SOURCE 200809L
#endif

This comment has been minimized.

Copy link
@ddevault

ddevault Aug 31, 2018

Member

No need to wrap this in ifndef

}

return EXIT_SUCCESS;
}

This comment has been minimized.

Copy link
@ddevault

ddevault Aug 31, 2018

Member

This whole file looks pretty wip

eglSwapBuffers(egl.display, egl_surface);
}

static void noop() {}

This comment has been minimized.

Copy link
@ddevault

ddevault Aug 31, 2018

Member

/* This space intentionally left blank */

void text_input_handle_leave(void *data,
struct zwp_text_input_v3 *zwp_text_input_v3,
struct wl_surface *surface) {
enabled = 2;

This comment has been minimized.

Copy link
@ddevault

ddevault Aug 31, 2018

Member

This variable is poorly named

} else if (strcmp(interface, zwp_text_input_manager_v3_interface.name) == 0) {
text_input_manager = wl_registry_bind(registry, name,
&zwp_text_input_manager_v3_interface, 1);

This comment has been minimized.

Copy link
@ddevault

ddevault Aug 31, 2018

Member

Extraneous newline

#include <stdlib.h>
#include <wayland-server.h>


This comment has been minimized.

Copy link
@ddevault

ddevault Aug 31, 2018

Member

Extra newline here and on line 3

struct wl_global *input_method_manager;
struct wl_resource *resource; // FIXME: turn into a list, each entry per-client

struct wlr_input_method *input_method; // FIXME: turn into a list, each entry per-seat

This comment has been minimized.

Copy link
@ddevault

ddevault Aug 31, 2018

Member

Can you fix this now?

This comment has been minimized.

Copy link
@dcz-purism

dcz-purism Sep 24, 2018

Author Contributor

The first FIXME pertains to a list of all input methods - since input methods are meant to take over inputs using grabs, does it make sense to allow multiple ones to be present at the same time?
(this doesn't actually change the shape of the struct, but still)
(after thinking a little, the answer is already in the code - each seat has a unique input method, and no one seems to have complained yet)

void *data) {
struct wlr_text_input *text_input = wl_container_of(listener, text_input,
seat_destroy_listener);
// wlr_text_input_destroy(text_input); TODO: what to do when the seat goes under?

This comment has been minimized.

Copy link
@ddevault

ddevault Aug 31, 2018

Member

become inert, probably

static void text_input_enable(struct wl_client *client,
struct wl_resource *resource) {
struct wlr_text_input *text_input = text_input_from_resource(resource);
struct wlr_text_input_properties defaults = {0};

This comment has been minimized.

Copy link
@Shugyousha

Shugyousha Sep 2, 2018

Contributor

This could be static.

.preedit_string = preedit_string,
.delete_surrounding_text = delete_surrounding_text,
.get_input_popup_surface = noop,
.grab_keyboard = noop,

This comment has been minimized.

Copy link
@Shugyousha

Shugyousha Sep 2, 2018

Contributor

I assume the noops here are still on the TODO list, correct?

This comment has been minimized.

Copy link
@dcz-purism

dcz-purism Sep 5, 2018

Author Contributor

Yes, I wanted to get the whole concept into shape before I tackle these. They can be placed on top of the current patch independently, which helps to avoid a huge change at once.

@Shugyousha

This comment has been minimized.

Copy link
Contributor

Shugyousha commented Sep 2, 2018


static void handle_unavailable(void *data,
struct zwp_input_method_v2 *zwp_input_method_v2) {
printf("IM disappeared");

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

Missing \n

#include "text-input-unstable-v3-client-protocol.h"
#include "xdg-shell-client-protocol.h"

#include <linux/input-event-codes.h>

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

Add in FreeBSD too. You can find examples of how to do this by grepping other examples.

#include "input-method-unstable-v2-client-protocol.h"
#include "xdg-shell-client-protocol.h"

#include <linux/input-event-codes.h>

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

FreeBSD compat

@@ -0,0 +1,73 @@
#ifndef WLR_TYPES_WLR_INPUT_METHOD_H

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

This file should have a _v2 suffix

@@ -0,0 +1,77 @@
#ifndef WLR_TYPES_WLR_TEXT_INPUT_H

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

This file should have a _v3 suffix (alongside with all structs).

Ah, and you need to add the WLR_USE_UNSTABLE stuff too at the beginning of all new header files.

};

struct wlr_text_input_manager {
struct wl_global *wl_global;

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

Should be named global


struct {
struct wl_signal text_input; // (struct wlr_text_input*)
} events;

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

Should have a destroy event

This comment has been minimized.

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

I mean a destroy event on the manager. The manager gets destroyed with the display. If the compositor has objects that depend on it it needs to destroy them with the manager (it can't destroy them with the display because the destroy listener of the manager may be triggered before the destroy listener of the objects, this has caused issues in the past).


struct wlr_text_input_manager {
struct wl_global *wl_global;
struct wl_list text_inputs; // wlr_text_input::resource::link

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

Hmm, you mean wlr_text_input::link?

#include <wlr/types/wlr_seat.h>
#include <wlr/types/wlr_surface.h>

struct wlr_text_input_properties {

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

We generally use state instead of properties

uint32_t anchor;
} surrounding;

uint32_t text_change_cause;

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

Can this be an enum? (applies to other fields too)

struct wlr_surface *focused_surface;
struct wlr_text_input_properties pending;
struct wlr_text_input_properties current;
uint32_t current_serial; // next to send

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

If it's the next serial, maybe we should rename to next_serial?

static void commit(struct wl_client *client, struct wl_resource *resource,
uint32_t serial) {
struct wlr_input_method *context =
wl_resource_get_user_data(resource);

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

Should add a safe wrapper input_method_from_resource for this

context->current_serial = serial;
struct wlr_input_method_state default_state = {0};
context->pending = default_state;
wlr_signal_emit_safe(&context->events.commit, (void*)context);

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

No need to cast to void *


static void input_method_handle_resource_destroy(struct wl_resource *resource) {
struct wlr_input_method *context = input_method_from_resource(resource);
if (!context) {

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

input_method_destroy should check for NULL

This comment has been minimized.

Copy link
@dcz-purism

dcz-purism Sep 24, 2018

Author Contributor

Does it apply to all destructors? The check was something I realized is not needed.

This comment has been minimized.

Copy link
@emersion

emersion Sep 24, 2018

Member

No, but that's how inert resources work. See the last part of CONTRIBUTING.md.

This comment has been minimized.

Copy link
@dcz-purism

dcz-purism Sep 24, 2018

Author Contributor

Thanks, I didn't realize this guide was there.

@dcz-purism

This comment has been minimized.

Copy link
Contributor Author

dcz-purism commented Sep 5, 2018

It would definitely be good to be able to do some testing with more complex input methods, i. e. ones that show word suggestions for example. Is the plan to add something like that to wlroots/examples/input-method.c?

Yes, I want to create additional examples for testing once I start working on the floating popup and keyboard grab interfaces.


static void manager_destroy(struct wl_client *client,
struct wl_resource *resource) {
// FIXME

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

This should call wl_resource_destroy

wl_list_remove(&text_input->link);
// no need to destroy the resource if client closed, prompting the server to do it
if (text_input->resource) {
wl_resource_destroy(text_input->resource);

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

We should never do this. Instead, the client should always send a request to destroy the resource.


static void text_input_destroy(struct wl_client *client,
struct wl_resource *resource) {
text_input_resource_destroy(resource);

This comment has been minimized.

Copy link
@emersion

emersion Sep 5, 2018

Member

This should just call wl_resource_destroy

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Sep 20, 2018

bump


// negative offsets already checked before
for (unsigned i = utf8_offset(preedit_text, current.preedit.cursor_begin);
i < utf8_offset(preedit_text, current.preedit.cursor_end); i++) {

This comment has been minimized.

Copy link
@ddevault

ddevault Oct 4, 2018

Member

Weird indent here

.preedit_string = im_preedit_string,
.delete_surrounding_text = im_delete_surrounding_text,
.get_input_popup_surface = noop, // TODO: implement
.grab_keyboard = noop, // TODO: implement

This comment has been minimized.

Copy link
@ddevault

ddevault Oct 4, 2018

Member

I'd rather have two real placeholder functions than one noop function

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Oct 4, 2018

This looks pretty good. I'd like to see the minor comments addressed and get a second opinion, then I'm in favor of merge.

@dcz-purism dcz-purism force-pushed the dcz-purism:input branch from 8f8dbae to 29f2f52 Oct 5, 2018

@dcz-purism

This comment has been minimized.

Copy link
Contributor Author

dcz-purism commented Oct 5, 2018

I addressed the issues and updated the input-method protocol to the new patch version to get rid of many mistakes in description texts. This implied one code change: preedit_string calls became set_preedit_string.

const char usage[] = "Usage: text-input [seconds [width height]]\n\
\n\
Creates a xdg-toplevel using the text-input protocol.\n\
It will be solid black when it has no text input focus, yellowwhen it\n\

This comment has been minimized.

Copy link
@Shugyousha

Shugyousha Oct 5, 2018

Contributor

"yellow when"

\n\
Creates a xdg-toplevel using the text-input protocol.\n\
It will be solid black when it has no text input focus, yellowwhen it\n\
has focus, and red when it was notified that the focused moved away\n\

This comment has been minimized.

Copy link
@Shugyousha

Shugyousha Oct 5, 2018

Contributor

"that the focus"

has focus, and red when it was notified that the focused moved away\n\
but still didn't give up the text input ability.\n\
\n\
The seconds argument is optional and defines the delay between getting\n\

This comment has been minimized.

Copy link
@Shugyousha

Shugyousha Oct 5, 2018

Contributor

"The second"

@dcz-purism dcz-purism force-pushed the dcz-purism:input branch from 29f2f52 to d45895d Oct 5, 2018

@dcz-purism

This comment has been minimized.

Copy link
Contributor Author

dcz-purism commented Oct 5, 2018

Fixed usage strings.

@dcz-purism

This comment has been minimized.

Copy link
Contributor Author

dcz-purism commented Oct 5, 2018

Did the build actually take place? There's not much in the logs...

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Oct 5, 2018

Restarted the build, seems to work better: https://builds.sr.ht/~emersion/job/8183

@emersion emersion self-requested a review Oct 5, 2018

cursor_mark[i] = '.';
}
if (current.preedit.cursor_begin == -1
&& current.preedit.cursor_end == -1) {

This comment has been minimized.

Copy link
@Shugyousha

Shugyousha Oct 6, 2018

Contributor

cursor_mark leaks here and at the other returns (where we don't goto end). Found by clang https://builds.sr.ht/~emersion/job/8183#task-clang-334 .

This comment has been minimized.

Copy link
@dcz-purism

dcz-purism Oct 9, 2018

Author Contributor

Fixed

dcz-purism added some commits Mar 29, 2018

wlroots: add basic support for zwp_input_method_v2
Implemented basic input method functionality. Not included: popups, grabbing.
rootston: add support for text input and the basic of input method
The compositor acts as a relay between applications using the text-input protocol and input methods using the input-method protocol.

This change implements the basic but useful support for input-method, leaving out grabs as well as popups.
build: bump wayland-scanner version
wayland-scanner >= 1.15.0 accepts foreign struct references, necessary in protocols like zwp-input-method-v2

@dcz-purism dcz-purism force-pushed the dcz-purism:input branch from d45895d to 585757d Oct 9, 2018

@dcz-purism

This comment has been minimized.

Copy link
Contributor Author

dcz-purism commented Oct 12, 2018

Bump

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Oct 12, 2018

Well, I like it. Thanks!

@ddevault ddevault merged commit 66e8908 into swaywm:master Oct 12, 2018

1 check passed

builds.sr.ht builds.sr.ht job completed successfully
Details
@Edootjuh

This comment has been minimized.

Copy link

Edootjuh commented Oct 12, 2018

It seems this merge has broken the build on an unused-result warning. Am I the only one?

[229/264] Compiling C object 'examples/examples@@input-method@exe/input-method.c.o'.
FAILED: examples/examples@@input-method@exe/input-method.c.o 
cc -Iexamples/examples@@input-method@exe -Iexamples -I../examples -I. -I../ -Iinclude -I../include -Iprotocol -I/usr/include/libdrm -I/usr/include/pixman-1 -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c11 -g -Wno-unused-parameter '-DWLR_SRC_DIR="/home/edo/.cache/yay/wlroots-git/src/wlroots-git"' -DWLR_USE_UNSTABLE -DWL_HIDE_DEPRECATED -march=x86-64 -mtune=generic -O2 -fstack-protector-strong -fno-plt -g -fvar-tracking-assignments -fdebug-prefix-map=/home/edo/.cache/yay/wlroots-git/src=/usr/src/debug -D_FORTIFY_SOURCE=2  -MD -MQ 'examples/examples@@input-method@exe/input-method.c.o' -MF 'examples/examples@@input-method@exe/input-method.c.o.d' -o 'examples/examples@@input-method@exe/input-method.c.o' -c ../examples/input-method.c
../examples/input-method.c: In function ‘main’:
../examples/input-method.c:393:4: error: ignoring return value of ‘read’, declared with attribute warn_unused_result [-Werror=unused-result]
    read(timer_fd, &expirations, sizeof(expirations));
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
[238/264] Compiling C object 'rootston/rootston@@rootston@exe/desktop.c.o'.
ninja: build stopped: subcommand failed.
@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Oct 12, 2018

Send a patch!

@dcz-purism

This comment has been minimized.

Copy link
Contributor Author

dcz-purism commented Oct 12, 2018

Thank you!

@dcz-purism

This comment has been minimized.

Copy link
Contributor Author

dcz-purism commented Oct 12, 2018

@Edootjuh I can't reproduce this, how are you compiling it?

@agx

This comment has been minimized.

Copy link
Contributor

agx commented Oct 12, 2018

@SirCmpwn thanks for fixing this up so quickly.

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Oct 12, 2018

@dcz-purism I pushed a fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.