Return-Path: Date: Thu, 14 Mar 2013 16:32:59 -0300 From: Vinicius Costa Gomes To: Alex Deymo Cc: linux-bluetooth@vger.kernel.org, keybuk@chromium.org, marcel@holtmann.org Subject: Re: [PATCH 2/6] Right prompt management on agent input Message-ID: <20130314193258.GB2649@samus> References: <1363285326-20089-1-git-send-email-deymo@chromium.org> <1363285326-20089-3-git-send-email-deymo@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1363285326-20089-3-git-send-email-deymo@chromium.org> List-ID: Hi Alex, On 11:22 Thu 14 Mar, Alex Deymo wrote: > Registering an agent shares the user input interface with the normal console > command interface. The way it is implemented (using rl_message, rl_save_prompt > and rl_restore_prompt) conflicts with the rl_printf calls that may appear > while waiting for user input, loosing the [bluetooth]# prompt. > This patch fixes this and makes clear if the expected input is a command or an > agent reply changing the color and text of the prompt. > --- > client/agent.c | 59 +++++++++++++++++++++++++++++++++++++++++++++----------- > client/display.h | 2 +- > client/main.c | 13 ++++++++----- > 3 files changed, 57 insertions(+), 17 deletions(-) > > diff --git a/client/agent.c b/client/agent.c > index b0ac2f8..fa8b1b2 100644 > --- a/client/agent.c > +++ b/client/agent.c > @@ -26,6 +26,7 @@ > #endif > > #include > +#include > #include > #include > > @@ -35,9 +36,47 @@ > #define AGENT_PATH "/org/bluez/agent" > #define AGENT_INTERFACE "org.bluez.Agent1" > > +#define AGENT_PROMPT COLOR_RED "[agent]" COLOR_OFF " " > + > static gboolean agent_registered = FALSE; > static const char *agent_capability = NULL; > static DBusMessage *pending_message = NULL; > +static char *agent_saved_prompt = NULL; > +static int agent_saved_point = 0; > + > +static void rl_agent_prompt(const char* msg) I don't like much using the "rl" prefix for local functions, it may confuse more than help. > +{ > + char *prompt; > + > + /* Normal use should not prompt for user input to the agent a second > + * time before it releases the prompt, but we take a safe action. */ > + if (agent_saved_prompt) > + return; > + > + agent_saved_point = rl_point; > + agent_saved_prompt = g_strdup(rl_prompt); > + > + prompt = g_strdup_printf(AGENT_PROMPT "%s", msg); > + rl_set_prompt(prompt); > + g_free(prompt); I would add an empty line here, don't know if it makes sense in the context of readline, but at least the code reads better. > + rl_replace_line("", 0); > + rl_redisplay(); > +} > + > +static void rl_agent_release_prompt(void) Same argument about the "rl" prefix. > +{ > + if (!agent_saved_prompt) > + return; > + > + /* This will cause rl_expand_prompt to re-run over the last prompt, but > + * our prompt doesn't expand anyway. */ > + rl_set_prompt(agent_saved_prompt); > + rl_replace_line("", 0); > + rl_point = agent_saved_point; > + rl_redisplay(); > + g_free(agent_saved_prompt); > + agent_saved_prompt = NULL; Again, if it makes sense, some empty lines may help readability. > +} > > dbus_bool_t agent_completion(void) > { > @@ -69,11 +108,11 @@ dbus_bool_t agent_input(DBusConnection *conn, const char *input) > { > const char *member; > > - rl_clear_message(); > - > if (!pending_message) > return FALSE; > > + rl_agent_release_prompt(); > + > member = dbus_message_get_member(pending_message); > > if (!strcmp(member, "RequestPinCode")) > @@ -97,9 +136,6 @@ dbus_bool_t agent_input(DBusConnection *conn, const char *input) > static DBusMessage *release_agent(DBusConnection *conn, > DBusMessage *msg, void *user_data) > { > - if (pending_message) > - rl_clear_message(); > - > agent_registered = FALSE; > agent_capability = NULL; > > @@ -110,6 +146,8 @@ static DBusMessage *release_agent(DBusConnection *conn, > pending_message = NULL; > } > > + rl_agent_release_prompt(); > + > g_dbus_unregister_interface(conn, AGENT_PATH, AGENT_INTERFACE); > > return dbus_message_new_method_return(msg); > @@ -125,7 +163,7 @@ static DBusMessage *request_pincode(DBusConnection *conn, > dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &device, > DBUS_TYPE_INVALID); > > - rl_message("Enter PIN code: "); > + rl_agent_prompt("Enter PIN code: "); > > pending_message = dbus_message_ref(msg); > > @@ -145,7 +183,7 @@ static DBusMessage *request_confirmation(DBusConnection *conn, > DBUS_TYPE_UINT32, &passkey, DBUS_TYPE_INVALID); > > str = g_strdup_printf("Confirm passkey %06u (yes/no): ", passkey); > - rl_message(str); > + rl_agent_prompt(str); > g_free(str); > > pending_message = dbus_message_ref(msg); > @@ -163,7 +201,7 @@ static DBusMessage *request_authorization(DBusConnection *conn, > dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &device, > DBUS_TYPE_INVALID); > > - rl_message("Accept pairing (yes/no): "); > + rl_agent_prompt("Accept pairing (yes/no): "); > > pending_message = dbus_message_ref(msg); > > @@ -182,7 +220,7 @@ static DBusMessage *authorize_service(DBusConnection *conn, > DBUS_TYPE_STRING, &uuid, DBUS_TYPE_INVALID); > > str = g_strdup_printf("Authorize service %s (yes/no): ", uuid); > - rl_message(str); > + rl_agent_prompt(str); > g_free(str); > > pending_message = dbus_message_ref(msg); > @@ -193,10 +231,9 @@ static DBusMessage *authorize_service(DBusConnection *conn, > static DBusMessage *cancel_request(DBusConnection *conn, > DBusMessage *msg, void *user_data) > { > - rl_clear_message(); > - > rl_printf("Request canceled\n"); > > + rl_agent_release_prompt(); > dbus_message_unref(pending_message); > pending_message = NULL; > > diff --git a/client/display.h b/client/display.h > index 9cb891a..957bdc6 100644 > --- a/client/display.h > +++ b/client/display.h > @@ -25,6 +25,6 @@ > #define COLOR_RED "\x1B[0;91m" > #define COLOR_GREEN "\x1B[0;92m" > #define COLOR_YELLOW "\x1B[0;93m" > -#define COLOR_BLUE "\x1B[0;34m" > +#define COLOR_BLUE "\x1B[0;94m" > > void rl_printf(const char *fmt, ...) __attribute__((format(printf, 1, 2))); > diff --git a/client/main.c b/client/main.c > index d8547c0..704cf46 100644 > --- a/client/main.c > +++ b/client/main.c > @@ -46,6 +46,9 @@ > #define COLORED_CHG COLOR_YELLOW "CHG" COLOR_OFF > #define COLORED_DEL COLOR_RED "DEL" COLOR_OFF > > +#define PROMPT_ON COLOR_BLUE "[bluetooth]" COLOR_OFF "# " > +#define PROMPT_OFF "[bluetooth]# " > + > static GMainLoop *main_loop; > static DBusConnection *dbus_conn; > > @@ -63,7 +66,7 @@ static void proxy_leak(gpointer data) > > static void connect_handler(DBusConnection *connection, void *user_data) > { > - rl_set_prompt(COLOR_BLUE "[bluetooth]" COLOR_OFF "# "); > + rl_set_prompt(PROMPT_ON); > printf("\r"); > rl_on_new_line(); > rl_redisplay(); > @@ -71,7 +74,7 @@ static void connect_handler(DBusConnection *connection, void *user_data) > > static void disconnect_handler(DBusConnection *connection, void *user_data) > { > - rl_set_prompt("[bluetooth]# "); > + rl_set_prompt(PROMPT_OFF); > printf("\r"); > rl_on_new_line(); > rl_redisplay(); > @@ -1283,11 +1286,11 @@ int main(int argc, char *argv[]) > rl_erase_empty_line = 1; > rl_callback_handler_install(NULL, rl_handler); > > - rl_set_prompt("[bluetooth]# "); > + rl_set_prompt(PROMPT_OFF); > rl_redisplay(); > > - input = setup_standard_input(); > - signal = setup_signalfd(); > + input = setup_standard_input(); > + signal = setup_signalfd(); > client = g_dbus_client_new(dbus_conn, "org.bluez", "/org/bluez"); > > g_dbus_client_set_connect_watch(client, connect_handler, NULL); > -- > 1.8.1.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, -- Vinicius