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.

Sign up for GitHub

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

Jump to bottom

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
Draft

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

wants to merge 30 commits into from

Conversation

LennysLounge
Copy link
Contributor

@LennysLounge LennysLounge commented Dec 30, 2023 •
edited
Loading

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
@Adanos020
Bump crate version
2b77fa0
@LennysLounge
Make all fields of a tree private
e031cc5 
-The DockState can use the method on the tree and does not need
to reach into the fields of a tree
@LennysLounge
Move node index methods into the tree
e0ba5c6 
- These operations are only possible on a binary tree and will
have to have access to the tree going forwards
@LennysLounge
Add child index to a parent node
c043d22
@LennysLounge
Make the tree non dependent on the binary tree
a64ecdf 
- 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
Reset the focused node when removing a node
b5c023e
@LennysLounge
Clear the nodes if the root is removed
d7dddb1
@LennysLounge
Correctly generate index iter when tree is empty
7e8613e
@LennysLounge
Use if let
1a42d66
@LennysLounge
Add changelog
ea84598
@LennysLounge
Iterate over tabs in breadth first order
5e9b23b
@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
@white-axe @Adanos020
Keyboard focus fixes (Adanos020#211)
ed990ed 
* 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
Merge branch 'release-0.10' into replace_binary_tree
b002a31
@Adanos020
Copy link
Owner

Adanos020 commented Jan 7, 2024 •
edited
Loading

@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
@Adanos020
Bump crate version
e4e5e19
@white-axe @Adanos020
Keyboard focus fixes (Adanos020#211)
c2066c5 
* 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>
@LennysLounge
Remove the empty node type
66865fd
@LennysLounge
Correctly set the focused node when moving a node
b0b60a3 
- 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
Improve documentation for the iter methods
3f8a797
@LennysLounge
Fix tests and make clippy happy
fd12ee3
@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.

@LennysLounge
A node now always has a rect
05594fb 
- 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
@LennysLounge LennysLounge mentioned this pull request Feb 11, 2024
Closed
@LennysLounge
Fix these todo
5ad83e4 
- The exact behavior of these functions still need to be discussed.
@LennysLounge
Use a simple hashmap to store nodes
63213ee
@LennysLounge
Store surfaces in a hashmap
8b3d088
@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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LennysLounge @Adanos020 @white-axe