switch_focus iterates from the last focusable line to _lineCount instead of the first line

[DarioMaechler]

Describe the bug
When iterating through the selected lines on a screen with next_screen() (forward/backward same issue), the last selected line is the line at _lineCount, except this line doesn't actually exist - so the cursor disappears for one line.

To Reproduce
After noticing the bug I tried the J_scrolling_menu-example, with the following changes:

• 20x4 screen --> screen.set_displayLineCount(4);
• I2C-library
  Bug occures in the example as well. Simply execute the function and watch the cursor disappear for 1 iteration.

Related code
// to solve the bug, I replaced all occurances of "_lineCount" in the following functions in
//LiquidScreen.cpp. I did NOT check if this breaks something else.
void LiquidScreen::switch_focus(bool forward) {
print_me(reinterpret_cast<uintptr_t>(this));
do {
if (forward) {
if (_focus < _lineCount -1) {
_focus++;
if (_focus == _lineCount-1) {
break;
}
} else {
_focus = 0;
}
} else { //else (forward)
if (_focus == 0) {
_focus = _lineCount-1;
break;
} else {
_focus--;
}
} //else (forward)
} while (_p_liquidLine[_focus]->_focusable == false);
DEBUG(F("Focus switched to ")); DEBUGLN(_focus);
}

Version of the library
V1.5.1

[thijstriemstra]

I also noticed this issue. Can you make a pull request of your fix @DarioMaechler?

[thijstriemstra]

Can you make a pull request of your fix @DarioMaechler?

I turned it into pull request #39 It can be installed into platformio until this is released (or fixed differently) using:

lib_deps =
  LiquidMenu=https://github.com/thijstriemstra/LiquidMenu/archive/patch-1.zip

Can you review the pull request @VaSe7u?

[VasilKalchev]

Hello, @DarioMaechler and @thijstriemstra.

This is intended by design for the purely aesthetic purpose of not showing the focus indicator when it's not used.
However you are not the first people to find this behavior confusing, so I might add an option to enable/disable it.

Thank's for the input.

[thijstriemstra]

Thanks for the feedback.

This is intended by design for the purely aesthetic purpose

I see. I think the default should not be the aesthetic version since it's confusing IMO, I thought my code/board was buggy. An option sounds good though.

[honolulu133]

Thanks to both of you as well.

I agree with @thijstriemstra. Based on the method names and the comments in the declaration I find the behavior confusing as well. Small example illustrating the problem:

1. execute switch_focus(true) until get_focusedLine() == _lineCount. Works fine so far (the focus just isn't visible).

2. execute set_focusedLine(get_focusedLine()) --> this will return false and not work

In my logic the getter returns an illegal value if the setter for the same item won't accept that same value.

[VasilKalchev]

Hello and sorry for the delay.

I thought about it and I agree that hiding the indicator while iterating the focusable lines really is confusing and this is not an intuitive place to put that functionality in.

I decided to make this side effect compile time configurable though a define inside src/LiquidMenu_config.h:
https://github.com/VaSe7u/LiquidMenu/blob/5978667585b9ea3cbdfcff5316bcb5b89fd10215/src/LiquidMenu_config.h#L83
It defaults to enabled in order to keep backwards compatibility.

Thank you for the input and the help with the code modification needed.