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

Add text tooltip for TabBar & TabContainer #89247

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

4d49
Copy link
Contributor

@4d49 4d49 commented Mar 7, 2024

@4d49 4d49 requested review from a team as code owners March 7, 2024 12:16
@AThousandShips AThousandShips added this to the 4.x milestone Mar 7, 2024
@akien-mga akien-mga changed the title Add tab tooltip text Add text tooltip for TabBar & TabContainer Mar 7, 2024
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.

LGTM

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Code looks pretty clean, and the feature makes sense. I'm surprised we didn't have it yet :D

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 7, 2024
scene/gui/tab_bar.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Mar 7, 2024

The tooltip overlaps preview of scene tabs:
image

@Mickeon
Copy link
Contributor

Mickeon commented Mar 7, 2024

I remember there was some discussion going in the past. Something about tooltips that only repeat the hovered subject being very annoying and superfluous. I wouldn't want to contribute to that by default as by this PR does (and as by KoBeWi's screenshot shows).

@YeldhamDev
Copy link
Member

I remember there was some discussion going in the past. Something about tooltips that only repeat the hovered subject being very annoying and superfluous.

I think those tooltips have a place when the tab's text is truncated, but yeah, it currently just shows it no matter what.

@ryevdokimov
Copy link
Contributor

ryevdokimov commented Mar 7, 2024

+1 to Mickeon's point. Tooltips will hijack 'ui_cancel' to close the tooltip. Select a node and then have your cursor over a tool mode button w/ the tooltip visible, then press escape (the default) to try to unselect the node, and you'll see what I mean.

I don't have an opinion on that specific behavior, but I do believe that it should be minimized, by not having tooltips showing redundant information scattered around the editor.

@Mickeon
Copy link
Contributor

Mickeon commented Mar 8, 2024

Point is, regarding this specific PR, the default behavior should be no tooltip at all (in my opinion)

@4d49
Copy link
Contributor Author

4d49 commented Mar 8, 2024

I reworked the code and now tooltips shows tab title only if text is truncated.

@Mickeon
Copy link
Contributor

Mickeon commented Mar 8, 2024

I think that makes sense, it's desirable and expected.
Are there cases in the editor where the tab title would appear truncated? Struggling to think of any right now, most tabs are not afraid to display their title in full.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 8, 2024

I'm not even sure how to make a tab truncated, as tabs simply disappear if they have no space.

@YeldhamDev
Copy link
Member

max_tab_width will limit how big a tab can get, and it's used in the editor for scene files with really long names:
image

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

The above code needs figuring out

@4d49 4d49 requested a review from kitbdev March 13, 2024 05:07
doc/classes/TabContainer.xml Show resolved Hide resolved
scene/gui/tab_bar.cpp Outdated Show resolved Hide resolved
Copy link
Member

@timothyqiu timothyqiu left a comment

Choose a reason for hiding this comment

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

The tooltip text should be carried over in TabContainer::move_tab_from_tab_container() so that dragging across different containers work.

@timothyqiu timothyqiu dismissed their stale review April 11, 2024 06:26

The requested changes are made.

Copy link
Member

@timothyqiu timothyqiu left a comment

Choose a reason for hiding this comment

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

Needs a rebase, otherwise looks good.

@4d49
Copy link
Contributor Author

4d49 commented Apr 23, 2024

@timothyqiu done!

@akien-mga akien-mga merged commit bb9457d into godotengine:master Apr 23, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@4d49 4d49 deleted the tab-tooltip branch April 24, 2024 03:13
@Lexpeartha
Copy link

Point is, regarding this specific PR, the default behavior should be no tooltip at all (in my opinion)

Also for the record, it doesn't seem possible to disable this behavior if you're using TabContainer since TabBar is built-in so you can't do anything about it. Not sure if this is desired behavior though

@Mickeon
Copy link
Contributor

Mickeon commented Jul 3, 2024

See:

So to me, if it's really just showing the tab's name, it's pretty undesirable and should be fixed.

@kitbdev
Copy link
Contributor

kitbdev commented Jul 3, 2024

Tooltips are only shown if the tab content is cut off, this only happens if the max_tab_width is set on the TabBar. This follows godotengine/godot-proposals#6235 (comment). To hide it then, the tooltip for each tab can be set to a space as mentioned in the docs for set_tab_tooltip.
But to get this to even happen on a TabContainer, you need to use get_tab_bar() to set the max_tab_width, so you have access to the TabBar and can disable it.

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.

Add text tooltip for TabBar & TabContainer
10 participants