Port swaylock to layer shell #1705

ddevault · 2018-04-03T03:16:02Z

Fixes #1506, fixes #1141, fixes #918, fixes #605, fixes #1072

ascent12 · 2018-04-03T20:05:58Z

common/meson.build

@@ -9,6 +9,7 @@ lib_sway_common = static_library(
 		'pango.c',
 		'readline.c',
 		'stringop.c',
+		'unicode.c',


Would it be possible to use mbrtoc32/c32rtomb from <uchar.h>, instead of doing it ourselves?

No, it's not portable.

They're standard C11 functions. I'm not certain, but they may introduce some locale-specific shit, though.

Looks like uchar.h is pretty shit after further research. It doesn't guarantee UTF-8 on the other side, just some locale-specific nonsense. I like my stock functions.

ddevault · 2018-04-03T23:16:34Z

This is ready for review.

acrisci · 2018-04-04T01:11:32Z

protocols/wlr-input-inhibitor-unstable-v1.xml

+      while the inhibitor is active.
+
+      The compositor may continue to send input events to selected clients,
+      such as an on-screen keyboard (via the input-method protocol).


what is the input-method protocol?

It's in wayland-protocols

I think you could qualify this a bit better.

acrisci · 2018-04-04T01:17:52Z

sway/input/input-manager.c

+		if (previous) {
+			wlr_log(L_DEBUG, "Returning focus to %p %s '%s'", previous,
+					container_type_to_str(previous->type), previous->name);
+			// Hack to get seat to re-focus the return value of get_focus


Because we handled focus out of the context of the seat's normal focus handling, seat_get_focus still returns what was focused before. Calling seat_set_focus with the return value of seat_get_focus (rightfully) exits early without doing anything, given that the previous focus is still valid. To work around this the hack changes the focus to the parent, then back again.

have you considered using the send_focus function?

it may be better, set focus to NULL.

I'm not 100% on the best way to refactor this, I would appreciate it if you filed a follow-up patch improving it if you're not happy with it

acrisci · 2018-04-04T01:20:50Z

sway/input/seat.c

@@ -350,6 +353,11 @@ void seat_configure_xcursor(struct sway_seat *seat) {
 		seat->cursor->cursor->y);
 }

+bool seat_allow_input(struct sway_seat *seat, struct wlr_surface *surface) {


functions that return bools should be like input_is_allowed or input_allowed. This sounds like it makes input allowed.

acrisci · 2018-04-04T01:23:42Z

sway/input/seat.c

+		for (int i = 0; i < root_container.children->length; ++i) {
+			struct sway_container *output = root_container.children->items[i];
+			if (!sway_assert(output->type == C_OUTPUT,
+						"root container has non-output child")) {


on i3, the scratchpad is a child of the root container.

on sway it won't be

It should be represented that way in the ipc get_tree or you'll break people's scripts.

acrisci · 2018-04-04T01:29:50Z

protocols/wlr-input-inhibitor-unstable-v1.xml

+      Clients can use this interface to prevent input events from being sent to
+      any surfaces but its own, which is useful for example in lock screen
+      software. It is assumed that access to this interface will be locked down
+      to whitelisted clients by the compositor.


I think you should specify what happens to input focus of the existing clients when this gets created (and destroyed).

Please move protocol feedback to the wlr-protocols thread

acrisci · 2018-04-04T01:37:47Z

sway/input/seat.c

+			seat_set_focus(seat, NULL);
+		}
+	}
+	if (seat->wlr_seat->pointer_state.focused_client) {


I don't want library users to dig into pointer_state. What about making a getter for this now that it's important?

acrisci · 2018-04-04T01:40:12Z

sway/input/seat.c

+	wl_list_for_each(point, &seat->wlr_seat->touch_state.touch_points, link) {
+		if (point->client->client != client) {
+			wlr_seat_touch_point_clear_focus(seat->wlr_seat,
+					now.tv_nsec / 1000, point->touch_id);


is this way of getting the time compatible with what we get from the real events?

acrisci · 2018-04-04T01:53:55Z

swaylock/render.c

+	wl_surface_attach(surface->surface, surface->current_buffer->buffer, 0, 0);
+	wl_surface_damage(surface->surface, 0, 0, buffer_width, buffer_height);
+	wl_surface_commit(surface->surface);
+	wl_display_roundtrip(state->display);


why are you roundtripping here?

sway/swaylock/password.c

Lines 102 to 108 in e79ef49

state->auth_state = AUTH_STATE_VALIDATING;

render_frames(state);

if (attempt_password(&state->password)) {

exit(0);

}

state->auth_state = AUTH_STATE_INVALID;

render_frames(state);

acrisci · 2018-04-04T02:00:43Z

It doesn't compile. I don't have rand().

acrisci · 2018-04-04T02:03:43Z

I don't have exit(). You should include stdlib.h.

emersion · 2018-04-04T19:03:55Z

swaylock/main.c

+		surface->image = background_image;
+
+		assert(surface->surface =
+				wl_compositor_create_surface(state.compositor));


No side effects in assertions

emersion · 2018-04-04T19:08:47Z

swaylock/password.c

+
+static bool backspace(struct swaylock_password *pw) {
+	if (pw->len != 0) {
+		pw->buffer[--pw->len] = 0;


Side note: with assignments in ifs, side effects in array brackets are practices I don't like and are error-prone. Especially for a lockscreen.

I don't think this is error prone, this behavior is well defined

I'm not saying this is undefined behavior, I'm just saying it's unreadable and thus error-prone, even for people used to these idioms. It's just like ifs without brackets.

Anyway, I don't care too much, this is just a warning to say "if there's a bug because of this, don't blame me for having approved this PR".

I know what you're saying. I disagree.

emersion · 2018-04-04T19:11:20Z

swaylock/password.c

+			render_frames(state);
+			wl_display_roundtrip(state->display);
+			if (attempt_password(&state->password)) {
+				exit(0);


Should cleanup all resources.

emersion · 2018-04-04T19:12:34Z

swaylock/render.c

+	struct swaylock_state *state = surface->state;
+	surface->current_buffer = get_next_buffer(state->shm,
+			surface->buffers,
+			surface->width * surface->scale,


Define buffer_width before this instruction, and replace surface->width * surface->scale with buffer_width

emersion · 2018-04-04T19:13:02Z

swaylock/render.c

+	}
+	cairo_identity_matrix(cairo);
+
+	int ARC_RADIUS = 50 * surface->scale;


Why are these in all caps?

Used to be constants. Will lowercase them now.

emersion · 2018-04-04T19:16:35Z

swaylock/render.c

+
+	wl_surface_set_buffer_scale(surface->surface, surface->scale);
+	wl_surface_attach(surface->surface, surface->current_buffer->buffer, 0, 0);
+	wl_surface_damage(surface->surface, 0, 0, buffer_width, buffer_height);


If you want to damage buffer_{width,height}, use wl_surface_damage_buffer. Or you could just use surface->{width,height}.

swaylock will use it too

inb4 acrisci

ddevault · 2018-04-05T00:18:12Z

Thanks!

ddevault added the hackathon label Apr 3, 2018

ddevault force-pushed the swaylock-layers branch 2 times, most recently from fec70c9 to fc2b037 Compare April 3, 2018 19:27

ascent12 reviewed Apr 3, 2018

View reviewed changes

ddevault force-pushed the swaylock-layers branch 5 times, most recently from 5beea9e to e79ef49 Compare April 3, 2018 23:15

ddevault changed the title ~~[WIP] Port swaylock to layer shell~~ Port swaylock to layer shell Apr 3, 2018

ddevault added the wlroots label Apr 3, 2018

acrisci reviewed Apr 4, 2018

View reviewed changes

acrisci self-requested a review April 4, 2018 02:16

ddevault force-pushed the swaylock-layers branch from 4c1deaa to cdac5ac Compare April 4, 2018 02:58

emersion reviewed Apr 4, 2018

View reviewed changes

ddevault added 19 commits April 4, 2018 18:47

Move swaybg background rendering into common/

a28730e

swaylock will use it too

Initial swaylock port

b32bf59

Grab keyboard off of the seat

402e1d9

Set up an XKB context for the keyboard

0bd40ce

Split seat code into its own file

1008d4c

Link swaylock to xkbcommon

1fe3cb8

Add password buffer, refactor rendering/surfaces

066143a

Verify passwords

e902de3

R E N D E R I N G

d053acb

Actually let's not do that TODO

62a736a

Refocus the last focused container on lock exit

3ede718

Add input inhibitor to input manager

06fbd51

Implement input-inhibit in sway, swaylock

b7e7794

Add hidpi support to swaylock

46b3889

Address review feedback

d2d050d

Move extra roundtrip into password.c

0138f79

Import stdlib.h and define POSIX macro for rand()

218a378

exit() needs stdlib.h

053b51c

inb4 acrisci

Address review feedback from @emersion

5d444b3

ddevault force-pushed the swaylock-layers branch from cdac5ac to 5d444b3 Compare April 4, 2018 22:52

emersion approved these changes Apr 5, 2018

View reviewed changes

emersion merged commit f2153f3 into wlroots Apr 5, 2018

emersion deleted the swaylock-layers branch April 5, 2018 00:16

	state->auth_state = AUTH_STATE_VALIDATING;
	render_frames(state);
	if (attempt_password(&state->password)) {
	exit(0);
	}
	state->auth_state = AUTH_STATE_INVALID;
	render_frames(state);

Port swaylock to layer shell #1705

Port swaylock to layer shell #1705

Conversation

ddevault commented Apr 3, 2018 • edited Loading

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

ddevault commented Apr 3, 2018

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

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

acrisci Apr 4, 2018 • edited Loading

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

acrisci commented Apr 4, 2018

acrisci commented Apr 4, 2018

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

Choose a reason for hiding this comment

ddevault commented Apr 5, 2018

ddevault commented Apr 3, 2018 •

edited

Loading

acrisci Apr 4, 2018 •

edited

Loading