Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manage inactive container selection #970

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

horrifyingHorse
Copy link

Hello there!

I had a similar issue as #935
What appears to be a reset is actually frame correctly identifying selected (by default) button in Container::Horizontal

So if our parent container contains a few buttons followed by a few horizontal containers of buttons, the scroll works fine until we pass by the first horizontal container, after which it will see the first horizontal container's button as selected.

This can be resolved by managing the state of selector_ based on whether the container is active or not so that the ActiveChild() is managed properly while rendering.

after managing selector:

#include <ftxui/component/component.hpp>
#include <ftxui/component/screen_interactive.hpp>

using namespace ftxui;

int main() {
  int size = 40;
  int size2 = 40;

  auto top_comp = Container::Vertical({});
  auto bottom_comp = Container::Vertical({});
  bottom_comp->Add(Button("exit", [] {}));

  for (int i = 0; i < 30; i++) {
    if (i % 2 == 0) {
      top_comp->Add(Button("Button #" + std::to_string(i), [] {}));
    } else {
      int* s = new int;
      *s = 1;
      auto hor_button_comp = Container::Horizontal({
          Button("Button #" + std::to_string(i), [] {}),
          Button("nottuB #" + std::to_string(i * 2), [] {}),
      }, s);
      top_comp->Add(hor_button_comp);
    }
  }

  auto rendered_top_comp = Renderer(top_comp, [&] {
    return top_comp->Render() | vscroll_indicator | hscroll_indicator | frame;
  });

  auto vert_split_comp =
      ResizableSplitTop(rendered_top_comp, bottom_comp, &size2);

  auto right_comp = Container::Horizontal({});
  right_comp->Add(Button("right split button 1", [] {}));
  right_comp->Add(Button("right split button 2", [] {}));

  auto hor_split_comp = ResizableSplitRight(right_comp, vert_split_comp, &size);

  auto screen = ScreenInteractive::Fullscreen();
  screen.Loop(hor_split_comp);
}
Screencast_20241216_214405.mp4

this will close #935

@ArthurSonzogni
Copy link
Owner

Thanks!

It seems to fix the problem from the linked bug, but I don't understand the solution.
Could you please help me understand it?

I believe we should reinvent the whole think from the ground up. Instead of 2-level of focus in between focus and selected decorator, we should have only one. To treat the priority, we should encode in hbox and vbox a way to prioritize one of the child based on a value provided.

@horrifyingHorse
Copy link
Author

horrifyingHorse commented Dec 22, 2024

Could you please help me understand it?

Sure!
The key issue - every time a Container is rendered, the active child of the component is found out with

Component ActiveChild() override {
  if (children_.empty()) {
    return nullptr;
  }
   return children_[static_cast<size_t>(*selector_) % children_.size()];
}

a component is returned as active regardless of Container itself being active
Example: if button is the child of a container

const bool active = Active(); 
...
auto focus_management = focused ? focus : active ? select : nothing;

the button is always set to active and then sets itself to select. This results the hbox to be selected in ComputeRequirement method, even if the Container is not active

here's a representation of what happens:

Screencast_20241223_004618.mp4

My Solution

  • We force the ContainerBase::ActiveChild to return nullptr when the Container itself is not active.
  • We do this while Render to ensure none of inactive Container's child is selected by ContainerBase::ActiveChild
  • Container::Tab is excluded as it needs to render tab's content regardless.

Instead of 2-level of focus in between focus and selected decorator, we should have only one.

If I;m not interpreting this incorrectly, you mean to get rid of either focus or select decorator entirely from FTXUI and re-implement the ComputeRequirement method?

In that case too we'll have to find a way so that the ContainerBase::ActiveChild returns a component only when it's necessary.

What do you think? I'll be happy to help!

@ArthurSonzogni
Copy link
Owner

Thanks!
I believe the proposed solution would only work when the children are direct child of the Container, but this might not always work when there are multiple nested Container.


There are 3 layers in FTXUI:

  1. ftxui/screen - a grid of pixels -> terminal output
  2. ftxui/dom - element to be drawn on the screen -> a grid of pixel
  3. ftxui/component - Interactive component -> elements to be drawn for a frame.

I believe the problem currently is that the "focusing" happens in both ftxui/dom and ftxui/component.


I believe we should modify Requirement::Selection from:

  enum Selection {
    NORMAL = 0,
    SELECTED = 1,
    FOCUSED = 2,
  };

To

  bool is_selected;

Then we should unite the decorator focus and selected into a single one that would set is_selected.

Then we should add hbox/vbox variants with a second argument, an index representing "how to merge" the Requirement. We should reimplement Hbox::ComputeRequirement to set is_selected based whether one child is selected, and set selected_box based on the selected box of the selected children.
If the selected children doesn't have any selected box, we must selected another one.

Finally, we should Modify Container::Horizontal and Container::Vertical to use the second version of hbox and vbox, plubming the currently selected children as the selected element.

What do you think?

@horrifyingHorse
Copy link
Author

Hello there!

Re-implementing ComputeRequirement with is_selected seems to solve the issue! I do think this is a better idea than what i did earlier.

I want to confirm the replacement of Requirement::Selection with is_selected as other elements like gridbox and flexbox seem to use it too!

Also i think we unite focus and select as select

@ArthurSonzogni
Copy link
Owner

Sounds good to me. I confirm both.
Please share your latest change, and I will apply final tweaks before merging it.

@horrifyingHorse
Copy link
Author

Please share your latest change

Sure!
I think we close this pr as the approach to bug fix completely changed

I will open a new pr with the updated code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Last scroll position of the vertical/horizontal container being reset when it's out of focus.
2 participants