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

Panic when retaining tabs #223

Closed
LennysLounge opened this issue Feb 11, 2024 · 1 comment · Fixed by #225
Closed

Panic when retaining tabs #223

LennysLounge opened this issue Feb 11, 2024 · 1 comment · Fixed by #225
Labels
bug Something isn't working

Comments

@LennysLounge
Copy link
Contributor

Describe the bug
When retaining tabs in a dock state the program will panic if all tabs are removed from a node.

To Reproduce
Run this program and click the "filter tabs" button in the menu bar.

#![cfg_attr(not(debug_assertions), windows_subsystem = "windows")] // hide console window on Windows in release

use eframe::{egui, NativeOptions};

use egui_dock::{DockArea, DockState, NodeIndex, Style};

fn main() -> eframe::Result<()> {
    let options = NativeOptions::default();
    eframe::run_native(
        "My egui App",
        options,
        Box::new(|_cc| Box::<MyApp>::default()),
    )
}

#[derive(Clone)]
enum Tab {
    Stay,
    GoAway,
}

struct TabViewer {}

impl egui_dock::TabViewer for TabViewer {
    type Tab = Tab;

    fn title(&mut self, tab: &mut Self::Tab) -> egui::WidgetText {
        match tab {
            Tab::Stay => "Stay".into(),
            Tab::GoAway => "GoAway".into(),
        }
    }

    fn ui(&mut self, ui: &mut egui::Ui, tab: &mut Self::Tab) {
        match tab {
            Tab::Stay => ui.label("This tab will stay"),
            Tab::GoAway => ui.label("This hab will go away"),
        };
    }
}

struct MyApp {
    tree: DockState<Tab>,
}

impl Default for MyApp {
    fn default() -> Self {
        let mut tree = DockState::new(vec![Tab::Stay]);
        let [_left, _right] =
            tree.main_surface_mut()
                .split_left(NodeIndex::root(), 0.5, vec![Tab::GoAway]);
        Self { tree }
    }
}

impl eframe::App for MyApp {
    fn update(&mut self, ctx: &egui::Context, _frame: &mut eframe::Frame) {
        egui::TopBottomPanel::top("Top panel").show(ctx, |ui| {
            if ui.button("Filter tabs").clicked() {
                //self.tree = self.tree.filter_tabs(|tab| matches!(tab, Tab::Stay));
                self.tree.retain_tabs(|tab| matches!(tab, Tab::Stay));
            }
        });
        DockArea::new(&mut self.tree)
            .style(Style::from_egui(ctx.style().as_ref()))
            .show(ctx, &mut TabViewer {});
    }
}

This bug happens because in the retain method of a tree, all Node::Empty are removed from the tree. This breaks the binary tree.
A similar thing happens when filtering instead. There the binary tree is not destroyed but the Node remains in the empty state and is displayed as a black hole in the tree. See image:

image

Expected behavior
For starters, retaining tabs shouldnt break the binary tree. However, that still leaves empty nodes visible in the layout. It should be easy enought to properly remove empty nodes from the binary tree, but this only really works if you have access to the tree in the first place.
The retain_tabs method on Node does not have access to the tree and cannot properly remove itself once empty.

I see two ways to solve this:

  1. Dont allow retain_tabs and filter_tabs on Nodes because it is not possible to properly restore the tree to a valid state.
  2. Allow Node::Leaf to have no tabs. In this case we should display a dummy empty tab that is always closeable. Closing this tab will then correclty remove it from the tree. I think this is a really good solution actually because filtering would then no longer impact the layout of the tree. This allows you to first filter any unwanted tabs and then add new ones without changing the layout and invalidating any node indicies you might have.
@LennysLounge LennysLounge added the bug Something isn't working label Feb 11, 2024
@LennysLounge
Copy link
Contributor Author

Not related to this bug but probably a good time to think about it.

With #212 Node::Empty and possibly Surface::Empty will be removed. Depending on what solution we go with this would impact the api of filter_tabs, filter_map_tabs and retain_tabs for Node and Surface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant