ArthurSonzogni / FTXUI

:computer: C++ Functional Terminal User Interface. :heart:
MIT License
6.94k stars 418 forks source link

[Bug] - Incorrect menu entry highlighting when multiple menus use same MenuOption #426

Closed ghost closed 1 year ago

ghost commented 2 years ago

Bug

In menu_multiple.cpp navigating between menus works as expected.

However, if you change lines 43-45 to initialize each Menu in the initializer list with the same MenuOption

  // This is buggy
  MenuOption opt;

  auto menu_global = Container::Vertical(
      {
          Window("Menu 1", Menu(&menu_entries[0], &menu_selected[0], &opt)),
          Window("Menu 2", Menu(&menu_entries[1], &menu_selected[1], &opt)),
          Window("Menu 3", Menu(&menu_entries[2], &menu_selected[2], &opt)),
      },
      &menu_selected_global);

Navigating between menus will result in the highlighting and the > to not be in sync until a subsequent keypress. I've attached a video of this behavior here.

A workaround

Using different opts works fine, but I would expect that I would be able to share one MenuOption between multiple menus:


  // This works
  MenuOption opt;
  MenuOption opt2;
  MenuOption opt3;

  auto menu_global = Container::Vertical(
      {
          Window("Menu 1", Menu(&menu_entries[0], &menu_selected[0], &opt)),
          Window("Menu 2", Menu(&menu_entries[1], &menu_selected[1], &opt2)),
          Window("Menu 3", Menu(&menu_entries[2], &menu_selected[2], &opt3)),
      },
      &menu_selected_global);
ArthurSonzogni commented 2 years ago

You are right! Thanks for reporting this!

avighnac commented 1 year ago

I'm able to replicate this issue, but can't understand the mechanism by which FTXUI components render their children. Why does the render function (in component.cpp) return an Element array? Which part of the code deals with the specific rendering of the foreground and '> '?

avighnac commented 1 year ago
struct EntryState {
  std::string label;  /// < The label to display.
  bool state;         /// < The state of the button/checkbox/radiobox
  bool active;        /// < Whether the entry is the active one.
  bool focused;       /// < Whether the entry is one focused by the user.
};

What's the difference between active and focused?

avighnac commented 1 year ago

I came up with a fix that works for the above mentioned issue, but fails two test cases, one of which is this.

TEST(MenuTest, AnimationsHorizontal) {
  ...
  selected = 1;
  {
    Screen screen(4, 3);
    Render(screen, menu->Render());
    EXPECT_EQ(
        screen.ToString(),
        "\x1B[7m1\x1B[27m \x1B[1m2\x1B[22m "
        "\r\n\x1B[97m\x1B[49m\xE2\x94\x80\x1B[90m\x1B["
        "49m\xE2\x95\xB6\xE2\x94\x80\xE2\x94\x80\x1B[39m\x1B[49m\r\n    ");
  }
  animation::Params params(2s);
  menu->OnAnimation(params);
  {
    Screen screen(4, 3);
    Render(screen, menu->Render());
    EXPECT_EQ(
        screen.ToString(),
        "\x1B[7m1\x1B[27m \x1B[1m2\x1B[22m "
        "\r\n\x1B[90m\x1B[49m\xE2\x94\x80\xE2\x95\xB4\x1B[97m\x1B["
        "49m\xE2\x94\x80\x1B[90m\x1B[49m\xE2\x95\xB6\x1B[39m\x1B[49m\r\n    ");
  }
}

Which is understandable, but what I did was change this (menu.cpp, around line 120):

      const bool is_focused = (focused_entry() == i) && is_menu_focused;
      const bool is_selected = (*selected_ == i);

to this:

      const bool is_focused = (selected_focus_ == i) && is_menu_focused;
      const bool is_selected = (*selected_ == i);

What's the difference between focused_entry() and selected_focus_?

I've noticed that the function focused_entry() returns 2 (instead of index 0) in the beginning (with this example), which causes the bug.