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 TabBar and TabContainer dragging issues #83637

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

kitbdev
Copy link
Contributor

@kitbdev kitbdev commented Oct 19, 2023

Removed code duplication between TabBar and TabContainer for dragging logic.
TabContainer now uses its TabBar for dragging functionality.

  • Fixed TabContainer drag preview not using icon minimum size (set through its TabBar).
  • Fixed TabBar drag preview not changing translation after dragging.
  • Fixed when dragging a tab to an empty TabBar, the tab cache is not updated, causing the tab to be drawn in the wrong area until updated.
  • Fixed when dragging a disabled tab to an empty TabBar, the tab_selected and tab_changed signals are emitted.
  • Fixed when dragging a tab to an empty TabBar, the tab_selected signal is emitted twice.
  • Fixed TabContainer not properly checking if its empty on dropping tabs (It's currently not really possible since it gets hidden without tabs, but still).
  • Changing docks with dock select now lets TabContainer handle the tab moving logic to reduce duplicate logic and make the code cleaner. It now also properly handles tab icons and metadata.
  • Removed xl_text from tab struct since it doesn't need to be stored there anymore.
  • TabContainer now just uses its TabBar's drag_to_rearrange_enabled.

@kitbdev kitbdev requested a review from a team as a code owner October 19, 2023 20:42
@YeldhamDev YeldhamDev added this to the 4.x milestone Oct 19, 2023
@YeldhamDev YeldhamDev self-assigned this Oct 20, 2023
@YeldhamDev
Copy link
Member

Fixed TabContainer drag preview not using icon minimum size (set through its TabBar).

I'm sorry, but I'm not quite sure what that means.

Fixed when dragging a tab to an empty TabBar, the tab cache is not updated, causing the tab to be drawn in the wrong area until updated.

I'm trying to replicate this bug, but I'm unable to. Could you give instructions?

Fixed TabBar drag preview not changing translation after dragging.

Same as above.

Fixed when dragging a disabled tab to an empty TabBar, the tab_selected and tab_changed signals are emitted.

I'm not quite so sure about this one. Technically the disabled tab is being selected, which would make the current behavior the correct one.

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 20, 2023

For 'Fixed TabContainer drag preview not using icon minimum size (set through its TabBar).'
Using code like this to set the icons minimum size:

tab_bar_a.set_tab_icon_max_width(0,20)
tab_container_a_2.set_tab_icon(0,TESTICON)
tab_container_a_2.get_tab_bar().set_tab_icon_max_width(0,20)

The minimum size is respected in the tab itself, but not in the drag preview:

godot_tab drab icon min size

For 'Fixed when dragging a tab to an empty TabBar, the tab cache is not updated, causing the tab to be drawn in the wrong area until updated.'

This happens when dragging a (non disabled) tab into an empty TabBar. Sometimes it seems to not happen because the tab is immediately hovered which updates the cache, but if you drag it into a spot where its not hovered immediately it happens. Also the tab needs to not be from the first position, since the x offset will be correct anyway.

godot_tab drag cache invalid

For 'Fixed TabBar drag preview not changing translation after dragging.'

I made sure localization was set up in my project and then I set up a keyboard shortcut to call TranslationServer.set_locale with different languages.
The TabContainer drag preview gets automatically updated to the new translation, but the TabBar preview does not.

godot_tab drag locale change

For 'Fixed when dragging a disabled tab to an empty TabBar, the tab_selected and tab_changed signals are emitted.'

I'm not quite so sure about this one. Technically the disabled tab is being selected, which would make the current behavior the correct one.

Disabled tabs never emit tab_selected, tab_changed, tab_clicked, or active_tab_rearranged when clicking or dragging in all other cases. With an exception of when the user calls set_current_tab on a disabled tab directly.
Currently there's guards like this in a couple places in the code, so it doesn't get set as current or emit those signals.

if (!is_tab_disabled(hover_now)) {
	set_current_tab(hover_now);
}

If disabled tabs should send these signals, we can change all the other behaviors so it is emitted.
The docs say that setting it disabled is "making it non-interactable." So maybe it shouldn't emit any signals. Or maybe only tab_clicked since it does currently emit tab_hovered? I'm not sure.

@YeldhamDev
Copy link
Member

YeldhamDev commented Oct 20, 2023

Using code like this to set the icons minimum size

Ah, I see now. We should really expose that function to TabContainer itself at one point.

The TabContainer drag preview gets automatically updated to the new translation, but the TabBar preview does not.

Strange, it's working fine on my end.

If disabled tabs should send these signals, we can change all the other behaviors so it is emitted.
The docs say that setting it disabled is "making it non-interactable." So maybe it shouldn't emit any signals. Or maybe only tab_clicked since it does currently emit tab_hovered? I'm not sure.

While disabling the tab_selected signal is desired (even if the tab isn't disabled, since dragging them in isn't a click or selection via script), my worry is that if we ever change TabContainer to allow tabs to be dragged into it while empty, the tab_changed signal would be wanted. Either that, or a new signal that indicates a tab has been dragged into it (which again, would need to be a PR of its own).
And I'd argue that tab_clicked should be emitted by disabled ones as well, but that should be a different PR.

BTW, I discovered a bug unrelated to this PR while testing stuff: when current_tab is set from the inspector, it's ignored at runtime. I assume it's because the tabs haven't been created yet?

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 21, 2023

I see what you mean about the disabled tab sending the tab_changed signal. When it is the only tab in a TabContainer it is shown even though it's disabled.
For instance, if a TabContainer has two tabs, one disabled and one not, and the not disabled tab is dragged out to a different TabContainer, the disabled one does emit the tab_changed signal.
So in this case a disabled tab dragged into an empty should emit tab_changed. I'll update the pr when I have time.

Strange, it's working fine on my end.

Hmm, are you using the default language as source strings for the translation? What was happening was the TabBar sets the drag preview to directly use the translated text. Since the drag preview automatically gets translated, the translated text can't be retranslated into a different language.

BTW, I discovered a bug unrelated to this PR while testing stuff: when current_tab is set from the inspector, it's ignored at runtime. I assume it's because the tabs haven't been created yet?

I'm getting this issue as well.
I don't know many details about how scenes are loaded, but it uses the setters right? Then set_current_tab would throw an error if trying to set out of range. There aren't many places that current is set though...

void TabBar::set_current_tab(int p_current) {
	ERR_FAIL_INDEX(p_current, get_tab_count());

	previous = current;
	current = p_current;

Also unrelated, I found a different bug as well: Updating (not setting) the icon texture doesn't update the minimum size or redraw. This can be seen in the editor by changing the texture's size. We should probably use connect_changed on the texture or something.

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 21, 2023

For the current tab bug, it might be related to #50930

@YeldhamDev
Copy link
Member

YeldhamDev commented Oct 21, 2023

Hmm, are you using the default language as source strings for the translation?

Yes, that's generally what's done when translating stuff with PO files. Now that I didn't I can reproduce it as well.

For the current tab bug, it might be related to #50930

Wow, older than I imagined. Thanks for finding the report!

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 22, 2023

Updated to emit a tab_changed signal when dragging a disabled tab into an empty TabBar.

@YeldhamDev
Copy link
Member

YeldhamDev commented Oct 22, 2023

Changes look good, however, good you do one last thing? TabContainer's description of the tab_selected signal is quite vague in comparison to TabBar's, could you copy the later one to the former in the docs? It's a small enough change that's still related to the topic that I think it's worth putting into this PR.

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 22, 2023

Changed TabContainer's tab_selected signal description to be the same as TabBar's.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Oct 23, 2023
@akien-mga akien-mga merged commit e2dc96b into godotengine:master Oct 23, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@kitbdev kitbdev deleted the tab-drag-fix branch October 23, 2023 16:06
OverloadedOrama added a commit to OverloadedOrama/godot-dockable-container that referenced this pull request Nov 11, 2023
Seems like Godot 4.2 changes a few things internally when it comes to TabContainers and as a result tabs could not be dragged and dropped into panels, only in other TabBars. This happened most likely due to this PR godotengine/godot#83637

I also added statements with the help of `Engine.get_version_info()` to preserve compatibility with Godot 4.1. If you have any code style recommendations, let me know!

Because Godot 4.2 is still in beta, perhaps we should wait until the stable version is out before merging this PR, in case they change stuff again.
OverloadedOrama added a commit to OverloadedOrama/godot-dockable-container that referenced this pull request Nov 11, 2023
Seems like Godot 4.2 changes a few things internally when it comes to TabContainers and as a result tabs could not be dragged and dropped into panels, only in other TabBars. This happened most likely due to this PR godotengine/godot#83637

I also added statements with the help of `Engine.get_version_info()` to preserve compatibility with Godot 4.1. If you have any code style recommendations, let me know!

Because Godot 4.2 is still in beta, perhaps we should wait until the stable version is out before merging this PR, in case they change stuff again.
@YuriSizov YuriSizov added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release and removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jan 24, 2024
@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 24, 2024

Cherry-picked for 4.1.4. (Mainly as a pre-requisite for #84122)

Edit: In the end, not cherry-picked due to other pre-requisite changes, making it quite convoluted to do cleanly (changes as far as #79341, for instance).

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

Successfully merging this pull request may close these issues.

4 participants