Input rework
Ray Strode
halfline at gmail.com
Wed Dec 3 12:50:53 PST 2008
Hi Charlie,
On IRC (irc.freenode.net, #plymouth) you asked me to review some patches
(which are here: http://cgit.freedesktop.org/plymouth/log/?h=input-rework)
that you're doing to allow for more flexibility with input.
Comments inline below (mostly nits that don't really matter):
> ============================================================
> Keystroke sensitifity
>
Should probably use better commit messages than this.
The usual convention is make the first line a summary saying
what you're doing, add a blank line, and then add a short
paragraph saying why you're doing it.
A good example here might be:
Add hooks to watch for keystrokes during boot up
There are cases during boot up where it's useful to check
for certain user input. We currently only handle asking
the user for a password. But we'd also like to handle, e.g.,
interactive startup scenarios. This commit puts the machinery
in place to accomodate those types of use cases.
>
> diff --git a/src/client/ply-boot-client.c b/src/client/ply-boot-client.c
> --- a/src/client/ply-boot-client.c
> +++ b/src/client/ply-boot-client.c
> @@ -570,6 +570,34 @@ ply_boot_client_ask_daemon_for_cached_passwords (ply_boot_client_t
> }
>
> void
> +ply_boot_client_ask_daemon_to_watch_for_keystroke (ply_boot_client_t *client,
> + const char *keys,
> + ply_boot_client_key_answer_handler_t handler,
> + ply_boot_client_response_handler_t failed_handler,
> + void *user_data)
> +{
Spacing here and in a few other places isn't right. things should line up.
"keys" should probably be called "keystroke" since the function name
is watch_for_keystroke
> diff --git a/src/client/ply-boot-client.h b/src/client/ply-boot-client.h
> --- a/src/client/ply-boot-client.h
> +++ b/src/client/ply-boot-client.h
> @@ -36,6 +36,10 @@ typedef void (* ply_boot_client_answer_handler_t) (void *user_data,
> const char *answer,
> ply_boot_client_t *client);
>
> +typedef void (* ply_boot_client_key_answer_handler_t) (void *user_data,
> + const char *answer,
> + ply_boot_client_t *client);
> +
Any particular reason you can't use the existing
ply_boot_client_answer_handler_t
type?
> @@ -147,20 +156,22 @@ answer_via_command (answer_state_t *answer_state,
> out:
> if (answer != NULL)
> close (command_input_sender_fd);
> - waitpid (pid, exit_status, 0);
> + if (exit_status) waitpid (pid, exit_status, 0);
> + else waitpid (pid, NULL, 0);
This change looks unneeded.
> static void
> -on_answer_failure (answer_state_t *answer_state)
> +on_answer_failure (answer_state_t *answer_state, ply_boot_client_t *client)
> {
> ply_event_loop_exit (answer_state->state->loop, 1);
> }
>
> static void
> on_answer (answer_state_t *answer_state,
> - const char *answer)
> + const char *answer,
> + ply_boot_client_t *client)
> {
> int exit_status;
Also, these changes seem unneeded (and below too for some other callbacks).
Were you getting a compiler warning?
If so, change is probably okay, or doing a cast at the caller side
would work, too.
Might be better to have this kind of clean up stuff as a separate commit though.
>
> @@ -199,6 +210,28 @@ on_answer (answer_state_t *answer_state,
> ply_event_loop_exit (answer_state->state->loop, WEXITSTATUS (exit_status));
> }
>
> +
> +static void
> +on_key_reply (answer_state_t *answer_state,
This should be a key_answer_state *, I think.
> + const char *answer,
> + ply_boot_client_t *client)
> +{
> +
> + if (answer_state->command != NULL)
> + {
> + bool command_started = false;
This variable isn't used, right?
> +
> + command_started = answer_via_command (answer_state, answer, NULL);
> + }
> + else
> + {
> + if (answer) write (STDOUT_FILENO, answer, strlen (answer));
> + }
> +
> + if (answer) ply_event_loop_exit (answer_state->state->loop, 0);
> + ply_event_loop_exit (answer_state->state->loop, 1);
> +}
The if statements above should probably have the condition on a separate line
from the meat. I don't want to harp on spacing issues though. Just
keep it in mind
when you're working, so the code looks consistent. Just makes it easier for new
comers to get acclumated to the code.
> exit_code = 0;
> @@ -379,6 +443,7 @@ main (int argc,
> "show-splash", "Show splash screen", PLY_COMMAND_OPTION_TYPE_FLAG,
> "hide-splash", "Hide splash screen", PLY_COMMAND_OPTION_TYPE_FLAG,
> "ask-for-password", "Ask user for password", PLY_COMMAND_OPTION_TYPE_FLAG,
> + "ignore-keystroke", "Remove sensitivity to a keystroke", PLY_COMMAND_OPTION_TYPE_STRING,
> "update", "Tell boot daemon an update about boot progress", PLY_COMMAND_OPTION_TYPE_STRING,
> "details", "Tell boot daemon there were errors during boot", PLY_COMMAND_OPTION_TYPE_FLAG,
> "wait", "Wait for boot daemon to quit", PLY_COMMAND_OPTION_TYPE_FLAG,
So, there is a bit of a mess here and it's my fault.
The plymouth client currently takes two types of commands
plymouth --command
and
plymouth subcommand --option1 --option2
the former method is deprecated. I'd like to move everything over to
the new format, since it's more flexible.
The only reason I haven't made the change over completely yet is
laziness. We should definitely not add new commands
of the first type though.
> --- a/src/libplybootsplash/ply-window.c
> +++ b/src/libplybootsplash/ply-window.c
> @@ -42,6 +42,7 @@
> #include "ply-buffer.h"
> #include "ply-event-loop.h"
> #include "ply-frame-buffer.h"
> +#include "ply-list.h"
> #include "ply-logger.h"
> #include "ply-utils.h"
>
> @@ -95,6 +96,12 @@
> #define TEXT_PALETTE_SIZE 48
> #endif
>
> +typedef struct
> +{
> + void *function;
I wouldn't use void * here for a function pointer. Instead,
create a typedef void (* ply_window_handler_t) (void *); and
use it.
> + void *user_data;
> +} ply_window_callback_t;
> +
I'd call this a ply_window_closure_t instead of
ply_window_callback_t (since it has both a callback and
data associated with the callback)
> @@ -257,8 +267,9 @@ process_keyboard_input (ply_window_t *window,
> size_t character_size)
> {
...
> - if (mbrtowc (&key, keyboard_input, 1, NULL) > 0)
> + if (mbrtowc (&key, keyboard_input, character_size, NULL) > 0)
Grr, that's embarassing. Should probably be fixed as a separate
commit though. In fact, looking at this again. I think it's
still wrong. mbrtowc returns size_t which is unsigned, so we need
to cast it to ssize_t I guess.
> +
> +void
> +ply_window_remove_keyboard_input_handler (ply_window_t *window,
> + ply_window_keyboard_input_handler_t input_handler)
> +{
> + ply_list_node_t *node;
> + assert (window != NULL);
> + for (node = ply_list_get_first_node(window->keyboard_input_handler_list); node; node = ply_list_get_next_node(window->keyboard_input_handler_list, node))
> + {
> + ply_window_callback_t *callback = ply_list_node_get_data (node);
> + if (callback->function == input_handler)
> + {
> + free(callback);
I'd probably have a ply_window_callback_free (well
ply_window_closure_free) function
here just for symmetry
> diff --git a/src/main.c b/src/main.c
> --- a/src/main.c
> +++ b/src/main.c
> @@ -48,6 +48,12 @@
> #define PLY_MAX_COMMAND_LINE_SIZE 512
> #endif
>
> +typedef struct
> +{
> + const char *keys;
> + ply_trigger_t *trigger;
> +} ply_keystroke_trigger_t;
Since this has-a trigger rather than is-a trigger, i'd probably call it
ply_keystroke_watch_t instead of ply_keystroke_trigger_t
>
> - ply_window_set_keyboard_input_handler (plugin->window, NULL, NULL);
> - ply_window_set_backspace_handler (plugin->window, NULL, NULL);
> - ply_window_set_enter_handler (plugin->window, NULL, NULL);
> + (plugin->window, NULL, NULL);
This looks fishy ^
> --- a/src/ply-boot-server.c
> +++ b/src/ply-boot-server.c
...
> +ply_boot_connection_on_keystroke_answer (ply_boot_connection_t *connection,
> + const char *key)
> +{
> +
> + uint8_t size;
> +
> + /* splash plugin isn't able to ask for password,
> + * punt to client
> + */
> + if (key == NULL)
> + {
> + if (!ply_write (connection->fd,
> + PLY_BOOT_PROTOCOL_RESPONSE_TYPE_NO_ANSWER,
> + strlen (PLY_BOOT_PROTOCOL_RESPONSE_TYPE_NO_ANSWER)))
> + ply_error ("could not write bytes: %m");
> + }
> + else
> + {
> + /* FIXME: support up to 4 billion (probably fine)
> + */
> + if (strlen (key) > 255)
> + ply_error ("password to long to fit in buffer");
> +
> + size = (uint8_t) strlen (key);
> +
> + if (!ply_write (connection->fd,
> + PLY_BOOT_PROTOCOL_RESPONSE_TYPE_ANSWER,
> + strlen (PLY_BOOT_PROTOCOL_RESPONSE_TYPE_ANSWER)) ||
> + !ply_write (connection->fd,
> + &size, sizeof (uint8_t)) ||
> + !ply_write (connection->fd,
> + key, size))
> + ply_error ("could not write bytes: %m");
> + }
> +
> +}
This looks cut-and-paste and still mentions things like "password" in it.
Should probably factor the code out into one function so the lame comments,
FIXME's don't get repeated.
> ============================================================
> Clearword entry and moving password entry to a higher level.
I don't like the name "clearword" should probably be
"plaintext entry", "text entry" or just "entry"
>
> diff --git a/src/client/ply-boot-client.c b/src/client/ply-boot-client.c
> --- a/src/client/ply-boot-client.c
> +++ b/src/client/ply-boot-client.c
> @@ -570,6 +570,21 @@ ply_boot_client_ask_daemon_for_cached_passwords (ply_boot_client_t
> }
>
> void
> +ply_boot_client_ask_daemon_for_clearword (ply_boot_client_t *client,
> +
> + const char *prompt,
> + ply_boot_client_answer_handler_t handler,
> + ply_boot_client_response_handler_t failed_handler,
> + void *user_data)
> +{
> + assert (client != NULL);
> +
> + ply_boot_client_queue_request (client, PLY_BOOT_PROTOCOL_REQUEST_TYPE_CLEARWORD,
maybe REQUEST_TYPE_QUESTION ?
> + prompt, (ply_boot_client_response_handler_t)
> + handler, failed_handler, user_data);
> +}
> +
> +void
> ply_boot_client_ask_daemon_to_watch_for_keystroke (ply_boot_client_t *client,
> const char *keys,
> ply_boot_client_key_answer_handler_t handler,
> diff --git a/src/client/ply-boot-client.h b/src/client/ply-boot-client.h
> --- a/src/client/ply-boot-client.h
> +++ b/src/client/ply-boot-client.h
> @@ -76,6 +76,11 @@ void ply_boot_client_ask_daemon_for_cached_passwords (ply_boot_client_t
> ply_boot_client_multiple_answers_handler_t handler,
> ply_boot_client_response_handler_t failed_handler,
> void *user_data);
> +void ply_boot_client_ask_daemon_for_clearword (ply_boot_client_t *client,
> + const char *prompt,
> + ply_boot_client_answer_handler_t handler,
> + ply_boot_client_response_handler_t failed_handler,
> + void *user_data);
> void ply_boot_client_ask_daemon_to_watch_for_keystroke (ply_boot_client_t *client,
> const char *keys,
> ply_boot_client_key_answer_handler_t handler,
> diff --git a/src/client/plymouth.c b/src/client/plymouth.c
> --- a/src/client/plymouth.c
> +++ b/src/client/plymouth.c
...
> +static void
> +on_clearword_reply (clearword_answer_state_t *answer_state,
> + const char *answer,
> + ply_boot_client_t *client)
> +{
> + if (answer_state->command != NULL)
> + {
> + bool command_started = false;
> +
> + command_started = answer_via_command (answer_state->command, answer, NULL);
not used variable
> static void
> +on_clearword_request (state_t *state,
> + const char *command)
> +{
> + char *prompt;
> + char *program;
> + int number_of_tries;
> + answer_state_t *answer_state;
should be clearword_answer_state_t. or maybe it should be the other way
around. keep this answer_state_t and make the other one password_state_t.
> + ply_command_parser_add_command (state.command_parser,
> + "ask-for-clearword", "Ask user for password",
maybe "ask-question", "Ask user a question" ?
> + (ply_command_handler_t)
> + on_clearword_request, &state,
> + "command", "Command to send clearword to via standard input",
> + PLY_COMMAND_OPTION_TYPE_STRING,
> + "prompt", "Message to display when asking for clearword",
> + PLY_COMMAND_OPTION_TYPE_STRING,
> + NULL);
> +
...
> diff --git a/src/libply/ply-buffer.c b/src/libply/ply-buffer.c
> --- a/src/libply/ply-buffer.c
> +++ b/src/libply/ply-buffer.c
> @@ -83,6 +83,7 @@ ply_buffer_remove_bytes (ply_buffer_t *buffer,
> buffer->size - bytes_to_remove);
> buffer->size -= bytes_to_remove;
> }
> + buffer->data[buffer->size] = '\0';
ugh, another embarrassing bug.
> }
>
> void
> @@ -94,6 +95,7 @@ ply_buffer_remove_bytes_at_end (ply_buffer_t *buffer,
> bytes_to_remove = MIN (buffer->size, bytes_to_remove);
>
> buffer->size -= bytes_to_remove;
> + buffer->data[buffer->size] = '\0';
and again
> @@ -182,16 +184,17 @@ ply_buffer_append_bytes (ply_buffer_t *buffer,
> assert (buffer != NULL);
> assert (bytes != NULL);
> assert (length != 0);
> -
> - if ((buffer->size + length) >= buffer->capacity)
> +
> + if (length >PLY_BUFFER_MAX_BUFFER_CAPACITY){
> + bytes += length - (PLY_BUFFER_MAX_BUFFER_CAPACITY-1);
bytes is void * here, probably better to cast to uint8_t *
before doing the pointer arithmetic
> + length = (PLY_BUFFER_MAX_BUFFER_CAPACITY-1);
> + }
> +
> + while ((buffer->size + length) >= buffer->capacity)
> {
> if (!ply_buffer_increase_capacity (buffer))
> {
> ply_buffer_remove_bytes (buffer, length);
> -
> - if ((buffer->size + length) >= buffer->capacity)
> - if (!ply_buffer_increase_capacity (buffer))
> - return;
> }
> }
>
> @@ -201,6 +204,7 @@ ply_buffer_append_bytes (ply_buffer_t *buffer,
> bytes, length);
>
> buffer->size += length;
> + buffer->data[buffer->size] = '\0';
> }
looks good. clearly, I should have added unit tests with this code.
> diff --git a/src/libplybootsplash/ply-boot-splash-plugin.h b/src/libplybootsplash/ply-boot-splash-plugin.h
> --- a/src/libplybootsplash/ply-boot-splash-plugin.h
> +++ b/src/libplybootsplash/ply-boot-splash-plugin.h
> @@ -33,7 +33,16 @@
>
> typedef struct _ply_boot_splash_plugin ply_boot_splash_plugin_t;
>
> -typedef void (* ply_boot_splash_password_answer_handler_t) (void *answer_data, const char *password);
> +typedef enum {
> + PLY_BOOT_SPLASH_DISPLAY_NORMAL,
> + PLY_BOOT_SPLASH_DISPLAY_CLEARWORD_ENTRY,
> + PLY_BOOT_SPLASH_DISPLAY_PASSWORD_ENTRY
> +} ply_boot_splash_display_type_t;
> +
> +typedef union {
> + struct {const char *prompt; const char *entry_text;} clearword;
> + struct {const char *prompt; int bullets;} password;
> +} ply_boot_splash_display_info_t;
>
> typedef struct
> {
> @@ -59,10 +68,9 @@ typedef struct
> void (* on_root_mounted) (ply_boot_splash_plugin_t *plugin);
> void (* hide_splash_screen) (ply_boot_splash_plugin_t *plugin,
> ply_event_loop_t *loop);
> -
> - void (* ask_for_password) (ply_boot_splash_plugin_t *plugin,
> - const char *prompt,
> - ply_trigger_t *answer);
> + void (* update_display) (ply_boot_splash_plugin_t *plugin,
> + ply_boot_splash_display_type_t type,
> + ply_boot_splash_display_info_t *info);
> void (* become_idle) (ply_boot_splash_plugin_t *plugin,
> ply_trigger_t *idle_trigger);
> } ply_boot_splash_plugin_interface_t;
I think I'd rather have ask_for_password and ask_question as separate
functions rather than using a union. There's no real reason for them
to be smooshed together.
> diff --git a/src/libplybootsplash/ply-entry.c b/src/libplybootsplash/ply-entry.c
> --- a/src/libplybootsplash/ply-entry.c
> +++ b/src/libplybootsplash/ply-entry.c
...
> @@ -162,59 +173,95 @@ ply_entry_draw (ply_entry_t *entry)
> ply_frame_buffer_fill_with_argb32_data (entry->frame_buffer,
> &entry->area, 0, 0,
> text_field_data);
> -
> - bullet_data = ply_image_get_data (entry->bullet_image);
> - bullet_area.width = ply_image_get_width (entry->bullet_image);
> - bullet_area.height = ply_image_get_height (entry->bullet_image);
> -
> - if (entry->number_of_bullets < entry->max_number_of_visible_bullets)
> - number_of_visible_bullets = entry->number_of_bullets;
> +
> + if (entry->is_password)
> + {
> + bullet_data = ply_image_get_data (entry->bullet_image);
> + bullet_area.width = ply_image_get_width (entry->bullet_image);
> + bullet_area.height = ply_image_get_height (entry->bullet_image);
> +
> + if (entry->number_of_bullets < entry->max_number_of_visible_bullets)
> + number_of_visible_bullets = entry->number_of_bullets;
> + else
> + {
> + number_of_visible_bullets = entry->max_number_of_visible_bullets;
> +
> + /* We've got more bullets than we can show in the available space, so
> + * draw a little half bullet to indicate some bullets are offscreen
> + */
> + bullet_area.x = entry->area.x;
> + bullet_area.y = entry->area.y + entry->area.height / 2.0 - bullet_area.height / 2.0;
> +
> + ply_frame_buffer_fill_with_argb32_data (entry->frame_buffer,
> + &bullet_area, bullet_area.width / 2.0, 0,
> + bullet_data);
> + }
> +
> + for (i = 0; i < number_of_visible_bullets; i++)
> + {
> + bullet_area.x = entry->area.x + i * bullet_area.width + bullet_area.width / 2.0;
> + bullet_area.y = entry->area.y + entry->area.height / 2.0 - bullet_area.height / 2.0;
> +
> + ply_frame_buffer_fill_with_argb32_data (entry->frame_buffer,
> + &bullet_area, 0, 0,
> + bullet_data);
> + }
> + }
> else
> {
> - number_of_visible_bullets = entry->max_number_of_visible_bullets;
> -
> - /* We've got more bullets than we can show in the available space, so
> - * draw a little half bullet to indicate some bullets are offscreen
> - */
> - bullet_area.x = entry->area.x;
> - bullet_area.y = entry->area.y + entry->area.height / 2.0 - bullet_area.height / 2.0;
> -
> - ply_frame_buffer_fill_with_argb32_data (entry->frame_buffer,
> - &bullet_area, bullet_area.width / 2.0, 0,
> - bullet_data);
This part shouldn't be needed here right?
> + ply_label_set_text (entry->label, entry->text);
> + ply_label_show (entry->label, entry->window, entry->area.x, entry->area.y);
> +
> }
> + ply_frame_buffer_unpause_updates (entry->frame_buffer);
> +}
Overall ply_entry_draw is getting too big for itself. I think i'd
split it out into two
helper functions, ply_entry_draw_text and ply_entry_draw_bullets
>
> - for (i = 0; i < number_of_visible_bullets; i++)
> +void
> +ply_entry_set_bullets (ply_entry_t *entry, int count)
call it set_number_of_bullets or set_bullet_count
> +{
> + count = MAX(0, count);
> + if (!entry->is_password || entry->number_of_bullets != count)
> {
> - bullet_area.x = entry->area.x + i * bullet_area.width + bullet_area.width / 2.0;
> - bullet_area.y = entry->area.y + entry->area.height / 2.0 - bullet_area.height / 2.0;
> -
> - ply_frame_buffer_fill_with_argb32_data (entry->frame_buffer,
> - &bullet_area, 0, 0,
> - bullet_data);
> + entry->is_password = true;
> + entry->number_of_bullets = count;
> + ply_entry_draw (entry);
> }
> - ply_frame_buffer_unpause_updates (entry->frame_buffer);
> +}
> +
> +int
> +ply_entry_get_bullets (ply_entry_t *entry)
get_number_of_bullets or get_bullet_count
> +++ b/src/libplybootsplash/ply-window.c
> --- a/src/main.c
> +++ b/src/main.c
> @@ -54,6 +54,15 @@ typedef struct
> ply_trigger_t *trigger;
> } ply_keystroke_trigger_t;
>
> +typedef struct
> +{
> + enum {PLY_ENTRY_TRIGGER_TYPE_PASSWORD,
> + PLY_ENTRY_TRIGGER_TYPE_CLEARWORD}
> + type;
> + const char *prompt;
> + ply_trigger_t *trigger;
> +} ply_entry_trigger_t;
> +
> typedef struct
> {
> ply_event_loop_t *loop;
> @@ -64,6 +73,8 @@ typedef struct
> ply_buffer_t *boot_buffer;
> ply_progress_t *progress;
> ply_list_t *keystroke_triggers;
> + ply_list_t *entry_triggers;
> + ply_buffer_t *entry_buffer;
> long ptmx;
I think input buffers should be per-window, since different windows
have different input devices associated with them. On the other hand,
we only need an answer from one given window before continuing...
It's sort of academic, since the only time we ever have more than one
window right now is if the user has a serial console.
> @@ -155,18 +167,28 @@ on_ask_for_password (state_t *state,
> const char *prompt,
> ply_trigger_t *answer)
> {
> - if (state->boot_splash == NULL)
> - {
> - show_detailed_splash (state);
> - if (state->boot_splash == NULL)
> - ply_trigger_pull (answer, "");
> - return;
> - }
> -
> - ply_boot_splash_ask_for_password (state->boot_splash,
> - prompt, answer);
> + ply_entry_trigger_t *entry_trigger =
> + calloc (1, sizeof (ply_entry_trigger_t));
> + entry_trigger->type = PLY_ENTRY_TRIGGER_TYPE_PASSWORD;
> + entry_trigger->prompt = prompt;
> + entry_trigger->trigger = answer;
> + ply_list_append_data (state->entry_triggers, entry_trigger);
> + update_display (state);
> }
Ahh, nice fix.
> dump_details_and_quit_splash (state_t *state)
> {
> state->showing_details = false;
> - on_escape_pressed (state);
> +/* on_escape_pressed (state); FIXME_brejc8 Check with halfline*/
So the reason we do:
show_details = false;
on_escape_pressed
is just to get it to show the details before quitting. I probably should have
factored out on_escape_pressed into common code between on_escape_pressed and
this function instead of munging with the state variable. That was laziness on
my part.
We need to see the details when quitting so, for example, if the root filesystem
doesn't get mounted we can show the error message from nash.
> --- a/src/plugins/splash/details/plugin.c
> +++ b/src/plugins/splash/details/plugin.c
> @@ -53,20 +53,17 @@
>
> #include <linux/kd.h>
>
> -void ask_for_password (ply_boot_splash_plugin_t *plugin,
> - const char *prompt,
> - ply_trigger_t *answer);
> +#define CLEAR_LINE_SEQUENCE "\033[2K\r"
> +
Shouldn't need this. call ply_window_clear_text_line instead.
Overall, looks good.
--Ray
More information about the plymouth
mailing list