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

Fix TabContainer drag to rearrange issue #83966

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

kitbdev
Copy link
Contributor

@kitbdev kitbdev commented Oct 25, 2023

TabContainer was using TabBar's dragging logic when drag to rearrange enabled was true, and it's own dragging logic when it was false.
Since it was using TabBar's drag_to_rearrange_enabled, it prevented proper forwarding to TabContainer.

@kitbdev kitbdev requested a review from a team as a code owner October 25, 2023 20:23
@YeldhamDev YeldhamDev added this to the 4.2 milestone Oct 25, 2023
Copy link
Member

@YeldhamDev YeldhamDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A blank line separating the if statements from the returns below would nice.

Also, I wonder if there's some C++ magic that would make the _handle_*_data() methods from TabBar only accessible to TabContainer, as having them be completely public leaves a bit of a sour taste in my mouth. 😛

@KoBeWi
Copy link
Member

KoBeWi commented Oct 25, 2023

A blank line separating the if statements from the returns below would nice.

I'd argue it's redundant here. } acts like a separator already.

Also, I wonder if there's some C++ magic that would make the handle*_data() methods from TabBar only accessible to TabContainer

Friend class. Although not sure if it can be used without including TabContainer in TabBar.

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 25, 2023

I'd argue it's redundant here. } acts like a separator already.

I'm fine with just the } and no blank line.
But if the blank lines are wanted, I could also put them _drag_move_tab_from and TabBar::get_drag_data.

Friend class. Although not sure if it can be used without including TabContainer in TabBar.

Yeah, I wanted to avoid putting TabContainer in TabBar.
A non-public, non-friend solution would have them be protected, and inherit a custom TabBar to use in TabContainer just to override the drag methods, but that's a lot just for drag overrides.

Copy link
Member

@YeldhamDev YeldhamDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh well, it's fine then.

@akien-mga akien-mga merged commit 71bef69 into godotengine:master Oct 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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