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

Replace binary tree to allow more that two splits per node #212

Draft
wants to merge 30 commits into
base: release-0.15
Choose a base branch
from

Conversation

LennysLounge
Copy link
Contributor

@LennysLounge LennysLounge commented Dec 30, 2023

Related to #147

Replaces the internal binary tree with a tree that allows a arbitrary amount of splits per node.

Adanos020 and others added 11 commits November 25, 2023 20:51
-The DockState can use the method on the tree and does not need
to reach into the fields of a tree
- These operations are only possible on a binary tree and will
have to have access to the tree going forwards
- Nodes are still stored in a Vec but the position and order of
the nodes no longer follows that of a binary tree.
A node still only allows for a left and right node. So in practice
it is still technically a binary tree. The implementation however
no longer relies on this fact and should make further changes
easer.

- A node can no longer be split on its own.
@LennysLounge
Copy link
Contributor Author

@Adanos020 In this first step i wanted to only remove any dependencies the code might have on the binary tree.
As a result of this, most methods on NodeIndex are not really possible and have been move into the tree itself.

The Tree still behaves like a binary tree but the layout of the nodes in the Vec is free. This should make it easier to implement the actual feature later.

white-axe and others added 2 commits December 31, 2023 19:22
* Fix crash after calling `DockState::remove_tab`. (Adanos020#208)

* Remove surface after removing the last remaining tab, if the surface isn't `Main`.

* Add explicit panic in `Tree::remove_leaf` if the `Tree` is empty.

* Bump patch version, update changelog.

* Include instructions on how to run examples in Readme. (Adanos020#209)

* Fix keyboard focus not working inside `DockArea`s

* Add visual indicators when tab buttons are focused

* Allow current tab to be switched using the keyboard

* Remove extra focusable areas

I removed focus from two widgets that are not supposed to be focusable.
Also, I added highlighting to the separators when they are focused
because apparently I can't remove the ability to focus them without
breaking the ability to drag the separators.

* Separators can now be moved with the arrow keys

* Add highlighting to active tabs that are keyboard focused

* Update changelog

* Add tab styles for keyboard-focused tabs

* Don't move separators unless Ctrl or Shift is down

This is so that using the arrow keys to change the keyboard focus will
still work normally.

* Clippy

---------

Co-authored-by: Adam Gąsior <adanos020@gmail.com>
@Adanos020
Copy link
Owner

Adanos020 commented Jan 7, 2024

@LennysLounge Hey, thanks for giving it a stab. I noticed that sometimes when you drag a tab out of a window surface and dock it somewhere else, the application panics on window_surface.rs, line 77. Turns out that it in the statement above, focused_leaf() returns an Empty node.

Since we're not operating on a binary tree anymore, maybe we should phase out Node::Empty completely, which should also fix this panic. Does that sound right?

@LennysLounge
Copy link
Contributor Author

Yes, Node::Empty only exists to fill out the empty spaces in the binary tree. No binary tree, no need for empty nodes. I was hesitant to do it at first since i wanted to keep these changes as small as possible. However, removing Node::Empty has to happen at some point anyway and it makes the code easier to understand if we remove it now.

That Bug you mentioned bother me though. I encountered a similar bug at the same place before and thought i had fixed it. Apparently not. And i cant replicate it from your description either. If i remove Node::Empty i might end up fixing the bug too but that doesnt sound very robust if you know what i mean. If you have some reliable way to replicated it i can take a look to fix it.

@Adanos020
Copy link
Owner

I think replacing the underlying data structure is already a very radical change, so going all in and removing Node::Empty makes sense. In 0.x releases we're free to make breaking changes like this without violating SemVer. 🙂

As for the bug, I found very a consistent reproduction:

  1. Run the simple example
  2. Pull any tab A out into a window
  3. Pull any other tab B and dock it in the previously created window, to the right of A
  4. Pull A out of the window and dock it anywhere in the main view
  5. Observe the application panicking

Adanos020 and others added 2 commits January 8, 2024 12:42
* Fix crash after calling `DockState::remove_tab`. (Adanos020#208)

* Remove surface after removing the last remaining tab, if the surface isn't `Main`.

* Add explicit panic in `Tree::remove_leaf` if the `Tree` is empty.

* Bump patch version, update changelog.

* Include instructions on how to run examples in Readme. (Adanos020#209)

* Fix keyboard focus not working inside `DockArea`s

* Add visual indicators when tab buttons are focused

* Allow current tab to be switched using the keyboard

* Remove extra focusable areas

I removed focus from two widgets that are not supposed to be focusable.
Also, I added highlighting to the separators when they are focused
because apparently I can't remove the ability to focus them without
breaking the ability to drag the separators.

* Separators can now be moved with the arrow keys

* Add highlighting to active tabs that are keyboard focused

* Update changelog

* Add tab styles for keyboard-focused tabs

* Don't move separators unless Ctrl or Shift is down

This is so that using the arrow keys to change the keyboard focus will
still work normally.

* Clippy

---------

Co-authored-by: Adam Gąsior <adanos020@gmail.com>
- When a node is removed this might cause a sibling of the node
to be moved. If the sibling is focused this caused the focused node
to point to an stale node.
@LennysLounge
Copy link
Contributor Author

Found the bug and fixed it. There was a special case when removing a node where a sibling node was moved. If that sibling node was focused, the focus was now on a stale node and could cause the panic. The focus is now correctly updated.

@LennysLounge
Copy link
Contributor Author

I successfuly removed Node::Empty. However that highlights a new issue. previously the Tree::iter and Tree::iter_mut methods simply iterated over the nodes vector with the empty nodes. Now there is no way to see that a node has been removed from the tree and you are looking at stale data.

I briefly tried to replace the iterator with one that only iterates over valid nodes in breadth first order but hit a blocker with the mutable iterator. I dont really know how to continue with that problem.

@Adanos020
Copy link
Owner

I see, I can take a look if there's a way to solve this.

- Make the breadth first index iterator public
@Adanos020 Adanos020 changed the base branch from release-0.10 to release-0.11 January 9, 2024 12:39
@Adanos020 Adanos020 changed the base branch from release-0.11 to release-0.12 February 6, 2024 19:43
- The exact behavior of these functions still need to be discussed.
@Adanos020 Adanos020 deleted the branch Adanos020:release-0.15 April 5, 2024 20:51
@Adanos020 Adanos020 closed this Apr 5, 2024
@Adanos020 Adanos020 reopened this Apr 5, 2024
@Adanos020 Adanos020 changed the base branch from release-0.12 to release-0.13 April 5, 2024 20:56
@Adanos020 Adanos020 deleted the branch Adanos020:release-0.15 July 3, 2024 18:40
@Adanos020 Adanos020 closed this Jul 3, 2024
@Adanos020 Adanos020 reopened this Jul 3, 2024
@Adanos020 Adanos020 changed the base branch from release-0.13 to release-0.14 July 3, 2024 19:00
@Adanos020 Adanos020 changed the base branch from release-0.14 to release-0.15 October 14, 2024 16:45
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.

3 participants