[2/2] ui/ncurses: Update keybindings for subsets
diff mbox

Message ID 20160712070339.3524-2-sam@mendozajonas.com
State Superseded
Headers show

Commit Message

Samuel Mendoza-Jonas July 12, 2016, 7:03 a.m. UTC
We now use KEY_LEFT and KEY_RIGHT for general navigation; update
subset_process_key() to use the following keybindings:

Reorder items up/down: Minus/Plus keys (-/+)
Delete item: Delete or Backspace

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
---
 ui/ncurses/nc-config-help.c |  6 ++++--
 ui/ncurses/nc-widgets.c     | 22 +++++++++++-----------
 2 files changed, 15 insertions(+), 13 deletions(-)

Comments

Jeremy Kerr July 12, 2016, 8:50 a.m. UTC | #1
Hi Sam,

> We now use KEY_LEFT and KEY_RIGHT for general navigation; update
> subset_process_key() to use the following keybindings:
> 
> Reorder items up/down: Minus/Plus keys (-/+)
> Delete item: Delete or Backspace

Neat!

> +/*
> + * ncurses doesn't define these and and KEY_MINUS is defined elsewhere;
> + * define some simple helpers for these
> + */
>  static bool key_is_minus(int key)
>  {
>  	return key == 055;
>  }

how about just

	return key == '-';

?

> +static bool key_is_plus(int key)
>  {
> -	return key == KEY_RIGHT;
> +	return key == 053;
>  }

and
	return key == '+';

In fact, do we need these helpers at all? We have direct comparisons
elsewhere in the code, and if we don't use magic octal numbers, then
it's clear what the comparisons do.

Cheers,


Jeremy
Murilo Opsfelder Araujo July 12, 2016, 3:20 p.m. UTC | #2
On 07/12/2016 04:03 AM, Samuel Mendoza-Jonas wrote:
[...]
> +/*
> + * ncurses doesn't define these and and KEY_MINUS is defined elsewhere;

There is an extra 'and'.
Samuel Mendoza-Jonas July 12, 2016, 10:31 p.m. UTC | #3
On Tue, 2016-07-12 at 16:50 +0800, Jeremy Kerr wrote:
> Hi Sam,
> 
> > 
> > We now use KEY_LEFT and KEY_RIGHT for general navigation; update
> > subset_process_key() to use the following keybindings:
> > 
> > Reorder items up/down: Minus/Plus keys (-/+)
> > Delete item: Delete or Backspace
> 
> Neat!
> 
> > 
> > +/*
> > + * ncurses doesn't define these and and KEY_MINUS is defined elsewhere;
> > + * define some simple helpers for these
> > + */
> >  static bool key_is_minus(int key)
> >  {
> >  	return key == 055;
> >  }
> 
> how about just
> 
> 	return key == '-';
> 
> ?
> 
> > 
> > +static bool key_is_plus(int key)
> >  {
> > -	return key == KEY_RIGHT;
> > +	return key == 053;
> >  }
> 
> and
> 	return key == '+';
> 
> In fact, do we need these helpers at all? We have direct comparisons
> elsewhere in the code, and if we don't use magic octal numbers, then
> it's clear what the comparisons do.

Aha, yes you're right. I think I was caught up in ncurses land (ie. patch
2), but yes at this point we're just comparing literal characters.
Thanks!

> 
> Cheers,
> 
> 
> Jeremy
> _______________________________________________
> Petitboot mailing list
> Petitboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/petitboot

Patch
diff mbox

diff --git a/ui/ncurses/nc-config-help.c b/ui/ncurses/nc-config-help.c
index 23bcd9d..a0cbb20 100644
--- a/ui/ncurses/nc-config-help.c
+++ b/ui/ncurses/nc-config-help.c
@@ -5,8 +5,10 @@  Autoboot: Specify which devices to autoboot from.\n"
 "\n"
 "By selecting the 'Add Device' button new devices can be added to the autoboot \
 list, either by UUID, MAC address, or device type. Once added to the boot \
-order, the priority of devices can be changed with the 'left' and 'right' keys \
-Devices can be individually removed from the boot order with the minus key. \
+order, the priority of devices can be changed with the '-' (minus) and \
+'+' (plus) keys. \
+Devices can be individually removed from the boot order with the 'delete' or \
+'backspace' keys. \
 Use this option if you have multiple operating system images installed.\n"
 "\n"
 "To autoboot from any device, select the 'Clear & Boot Any' button. \
diff --git a/ui/ncurses/nc-widgets.c b/ui/ncurses/nc-widgets.c
index c5c4cec..f2b679a 100644
--- a/ui/ncurses/nc-widgets.c
+++ b/ui/ncurses/nc-widgets.c
@@ -162,19 +162,18 @@  static bool key_is_select(int key)
 	return key == ' ' || key == '\r' || key == '\n' || key == KEY_ENTER;
 }
 
+/*
+ * ncurses doesn't define these and and KEY_MINUS is defined elsewhere;
+ * define some simple helpers for these
+ */
 static bool key_is_minus(int key)
 {
 	return key == 055;
 }
 
-static bool key_is_left(int key)
-{
-	return key == KEY_LEFT;
-}
-
-static bool key_is_right(int key)
+static bool key_is_plus(int key)
 {
-	return key == KEY_RIGHT;
+	return key == 053;
 }
 
 static bool process_key_nop(struct nc_widget *widget __attribute__((unused)),
@@ -522,7 +521,8 @@  static bool subset_process_key(struct nc_widget *w, FORM *form, int key)
 	int i, val, opt_idx = -1;
 	FIELD *field;
 
-	if (!key_is_minus(key) && !key_is_left(key) && !key_is_right(key))
+	if (!key_is_minus(key) && !key_is_plus(key) &&
+		key != KEY_DC && key != KEY_BACKSPACE)
 		return false;
 
 	field = current_field(form);
@@ -538,10 +538,10 @@  static bool subset_process_key(struct nc_widget *w, FORM *form, int key)
 	if (opt_idx < 0)
 		return false;
 
-	if (key_is_minus(key))
+	if (key == KEY_DC || key == KEY_BACKSPACE)
 		subset_delete_active(subset, opt_idx);
 
-	if (key_is_left(key)){
+	if (key_is_minus(key)) {
 		if (opt_idx == 0)
 			return true;
 
@@ -550,7 +550,7 @@  static bool subset_process_key(struct nc_widget *w, FORM *form, int key)
 		subset->active[opt_idx - 1] = val;
 	}
 
-	if (key_is_right(key)){
+	if (key_is_plus(key)) {
 		if (opt_idx >= subset->n_active - 1)
 			return true;