Add increment and decrement controls for numeric display options

[ahmedaabouzied]

• Display controls to increment & decrement the numeric options.
• Display control to select/deselect non-numeric options with the enter key.
• This allows incrementing and decrementing values with the mouse.

Resolves: #737

Screenshot after applying the change:

[Explorer09]

• The increment button should be on the right and the decrement on the left. Example picture from Nielsen Norman Group There's a UI design guideline for such a numeric control.
• Hide the increment and decrement buttons on non-numeric controls. It's more confusing when a boolean control is highlighted and user tries increment and decrement buttons and found out they don't work.
• I don't like the "(De)Select" shorthand. Just label it "Select".

[ahmedaabouzied]

I've updated the PR in response to @Explorer09 comments with the following changes:

• Renames the (De)select button name to Select.
• Makes the Increment button on the right of the Decrement button.
• Shows two different function bars according to the type of item selected.
  • It will show a numeric function bar with Decrement and Increment buttons with numeric items.
  • It will show a boolean function bar with the Select button with boolean items.

[ahmedaabouzied]

@BenBE I think my editor is messing up the indentation. Is there a specific linter I should run?

[BenBE]

@BenBE I think my editor is messing up the indentation. Is there a specific linter I should run?

There is a rough command line mentioned in the STYLEGUIDE using astyle. Be sure to only include the lines near the code you edited.

[ahmedaabouzied]

There is a rough command line mentioned in the STYLEGUIDE using astyle. Be sure to only include the lines near the code you edited.

@BenBE , I think just manually fixed the indentation issues here. Could you take another look?

[BenBE]

There is a rough command line mentioned in the STYLEGUIDE using astyle. Be sure to only include the lines near the code you edited.

@BenBE , I think just manually fixed the indentation issues here. Could you take another look?

Quick look shows still some issues left. Check for 3 spaces per level.

[ahmedaabouzied]

Quick look shows still some issues left. Check for 3 spaces per level.

Ok, 3 spaces it is.

[BenBE]

Another small adjustment:

$ git diff --cached 
diff --git a/DisplayOptionsPanel.c b/DisplayOptionsPanel.c
index 42489077..176b017e 100644
--- a/DisplayOptionsPanel.c
+++ b/DisplayOptionsPanel.c
@@ -27,8 +27,6 @@ static const char* const NumericDisplayOptionsFunctions[] = {"Decrement ", "Incr
 static const char* const NumericDisplayOptionsKeys[] = {"-", "+", "F10", "Esc"};
 static const int NumericDisplayOptionsEvents[] = {'-', '+', KEY_F(10), 27};
 
-
-
 static void DisplayOptionsPanel_delete(Object* object) {
    Panel* super = (Panel*) object;
    DisplayOptionsPanel* this = (DisplayOptionsPanel*) object;
@@ -54,15 +52,15 @@ static HandlerResult DisplayOptionsPanel_eventHandler(Panel* super, int ch) {
 
    switch (ch) {
 
-   case KEY_UP:{
+   case KEY_UP: {
       int selected_index = Panel_getSelectedIndex(super);
       if (selected_index > 1) {
          OptionItem* next_to_select = (OptionItem*) Panel_get(super, selected_index -1);
          DisplayOptionsPanel_setFunctionBar(super, next_to_select);
       }
       break;
-    }
-   case KEY_DOWN:{
+   }
+   case KEY_DOWN: {
       int selected_index = Panel_getSelectedIndex(super);
       if (selected_index < Panel_size(super) - 1) {
          OptionItem* next_to_select = (OptionItem*) Panel_get(super, selected_index +1);

[ahmedaabouzied]

@BenBE Alright.

[ahmedaabouzied]

I resolved to format the file with asyle as stated in the /doc/styleguide.md file which messed things even further. This is not fun XD

[ahmedaabouzied]

@BenBE , sorry for all that mess. I think now it's reasonably formatted.

[ahmedaabouzied]

@BenBE , now initially the Panel is initiated with an empty function bar, then the right function bar is set based on the default selected item using the DisplayOptionsPanel_setFunctionBar() function. This will make it so that if that first item type is changed, the function bar will change with it.

Let me know if you have more comments. Thanks.

[ahmedaabouzied]

I think this fixes the memory leak @BenBE , but I'm not sure how this should be altered given the PR #772. What should be changed here to work with the changes in #772 ?

[BenBE]

The changes in #772 simplify this PR such that you just have to care about allocating the three function bars on initialization and freeing them on exit. Thus your extra two function bars for numeric and boolean values need to be allocated on init and freed on delete. Your default function bar, if neither is selected is managed by the panel code.

The second change (which should make the code for this PR even simpler) is that you only ever have to set the proper function bar for each item, without need to free it. If neither the numeric nor boolean option applies you simply can set the displayed function bar to NULL and everything is fine.

Over at this comment above I suggested an rename. Optional to do, just give some feedback about it if you want to not act on it.

[ahmedaabouzied]

Over at this comment above I suggested an rename. Optional to do, just give some feedback about it if you want to not act on it.

@BenBE , renamed that.

[ahmedaabouzied]

hey @BenBE , does this need any further modifications?

[BenBE]

AFAIR the latest version of the PR was okay as-is. But as 3.1.1 is for bugfixes only (without new features where possible) this has been scheduled for 3.2.0. So unless there appears a merge issue due to new upstream patches there's nothing to be done for this PR right now.

There's one minor thing you might have a look into though: With #772 the handling of the function bars might get even easier. Unfortunately last time I tested #772 there was still some segfault I couldn't quite wrap my head around. So if you like you could check there if you notice anything odd.

[BenBE]

Given there's #822 on the list of likely extensions, it's good planning to go the switch statement route directly.

[BenBE]

Hi @ahmedaabouzied, any updates with this PR? Need help with any of the issues raised?

[ahmedaabouzied]

Hi @ahmedaabouzied, any updates with this PR? Need help with any of the issues raised?

Sorry @BenBE , I missed the notification there. I've applied the changes you requested above.

[cgzones]

I think one can re-use the drawFunctionBar callback of the Panel class, which avoids the KEY_UP/DOWN interception:

diff --git a/DisplayOptionsPanel.c b/DisplayOptionsPanel.c
index 8212120..0e3ac03 100644
--- a/DisplayOptionsPanel.c
+++ b/DisplayOptionsPanel.c
@@ -20,11 +20,18 @@ in the source distribution for its full text.
 #include "ProvideCurses.h"
 
 
-static const char* const DisplayOptionsFunctions[] = {"      ", "      ", "      ", "      ", "      ", "      ", "      ", "      ", "      ", "Done  ", NULL};
+static const char* const CheckboxDisplayOptionsFunctions[] = {"Select  ", "Done  ", NULL};
+static const char* const CheckboxDisplayOptionsKeys[] = {"Enter", "F10"};
+static const int CheckboxDisplayOptionsEvents[] = {KEY_ENTER, KEY_F(10)};
+static const char* const NumericDisplayOptionsFunctions[] = {"Decrement ", "Increment ", "Done  ", NULL};
+static const char* const NumericDisplayOptionsKeys[] = {"-", "+", "F10"};
+static const int NumericDisplayOptionsEvents[] = {'-', '+', KEY_F(10)};
 
 static void DisplayOptionsPanel_delete(Object* object) {
    Panel* super = (Panel*) object;
    DisplayOptionsPanel* this = (DisplayOptionsPanel*) object;
+   FunctionBar_delete(this->numericFuBar);
+   FunctionBar_delete(this->checkboxFuBar);
    Panel_done(super);
    free(this);
 }
@@ -79,22 +86,46 @@ static HandlerResult DisplayOptionsPanel_eventHandler(Panel* super, int ch) {
    return result;
 }
 
+static void DisplayOptionsPanel_drawFunctionBar(Panel* super, ATTR_UNUSED bool hideFunctionBar) {
+   DisplayOptionsPanel* this = (DisplayOptionsPanel*) super;
+
+   int selected_index = Panel_getSelectedIndex(super);
+   OptionItem* item = (OptionItem*) Panel_get(super, selected_index);
+
+   switch (OptionItem_kind(item)) {
+      case OPTION_ITEM_NUMBER:
+         super->currentBar = this->numericFuBar;
+         break;
+      case OPTION_ITEM_CHECK:
+         super->currentBar = this->checkboxFuBar;
+         break;
+      default:
+         assert(0); // Unknown option type
+   }
+   FunctionBar_draw(super->currentBar);
+}
+
 const PanelClass DisplayOptionsPanel_class = {
    .super = {
       .extends = Class(Panel),
       .delete = DisplayOptionsPanel_delete
    },
-   .eventHandler = DisplayOptionsPanel_eventHandler
+   .eventHandler = DisplayOptionsPanel_eventHandler,
+   .drawFunctionBar = DisplayOptionsPanel_drawFunctionBar
 };
 
 DisplayOptionsPanel* DisplayOptionsPanel_new(Settings* settings, ScreenManager* scr) {
    DisplayOptionsPanel* this = AllocThis(DisplayOptionsPanel);
    Panel* super = (Panel*) this;
-   FunctionBar* fuBar = FunctionBar_new(DisplayOptionsFunctions, NULL, NULL);
-   Panel_init(super, 1, 1, 1, 1, Class(OptionItem), true, fuBar);
+   FunctionBar* checkboxFuBar = FunctionBar_new(CheckboxDisplayOptionsFunctions, CheckboxDisplayOptionsKeys, CheckboxDisplayOptionsEvents);
+   FunctionBar* numericFuBar = FunctionBar_new(NumericDisplayOptionsFunctions, NumericDisplayOptionsKeys, NumericDisplayOptionsEvents);
+   FunctionBar* emptyFuBar = FunctionBar_new(NULL, NULL, NULL);
+   Panel_init(super, 1, 1, 1, 1, Class(OptionItem), true, emptyFuBar);
 
    this->settings = settings;
    this->scr = scr;
+   this->numericFuBar = numericFuBar;
+   this->checkboxFuBar = checkboxFuBar;
 
    Panel_setHeader(super, "Display options");
    Panel_add(super, (Object*) CheckItem_newByRef("Tree view", &(settings->treeView)));
diff --git a/DisplayOptionsPanel.h b/DisplayOptionsPanel.h
index 5e87a63..899cf3c 100644
--- a/DisplayOptionsPanel.h
+++ b/DisplayOptionsPanel.h
@@ -17,6 +17,8 @@ typedef struct DisplayOptionsPanel_ {
 
    Settings* settings;
    ScreenManager* scr;
+   FunctionBar* numericFuBar;
+   FunctionBar* checkboxFuBar;
 } DisplayOptionsPanel;
 
 extern const PanelClass DisplayOptionsPanel_class;

[macmbello]

#the manage the upskill od the main menu we have to atleat give the meaning of the menu's bar