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

Support updating tooltip immediately after editor description change #82916

Conversation

jsjtxietian
Copy link
Contributor

@jsjtxietian jsjtxietian commented Oct 6, 2023

Should fix #82896

cc @Calinou

@jsjtxietian jsjtxietian requested review from a team as code owners October 6, 2023 14:23
scene/gui/tree.cpp Outdated Show resolved Hide resolved
scene/gui/tree.cpp Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

You've linked the wrong issue it looks like

@jsjtxietian jsjtxietian force-pushed the update-scene-tree-tooltip-after-editor-description-chnage branch from 2bd7c9a to c24dd27 Compare October 6, 2023 14:28
@jsjtxietian
Copy link
Contributor Author

You've linked the wrong issue it looks like

Thanks! The github's auto complete for issue number is terrible.

@jsjtxietian jsjtxietian force-pushed the update-scene-tree-tooltip-after-editor-description-chnage branch 2 times, most recently from f9d6d3f to d035392 Compare October 7, 2023 06:16
@jsjtxietian jsjtxietian requested a review from a team as a code owner October 7, 2023 06:16
@KoBeWi
Copy link
Member

KoBeWi commented Nov 7, 2023

Instead of adding a new signal, you could reuse node_configuration_warning_changed signal. It does update the whole tree, but it's connected to a Timer, so it shouldn't stall while writing the description, only once finished writing. Also only very big scenes would be really affected (thousands of nodes), so it's acceptable I think.

@jsjtxietian
Copy link
Contributor Author

It does update the whole tree

I'm a little worried about this as I prefer to not affect performance by this small commit. IMHO a little more code is fine compared to potential performance stalls, if we believe more and more people will use godot to make games with complex scenes. And it's not a configuration warning, so I think a seperate signal is fine. Feel free to correct me.

@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Nov 8, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Nov 8, 2023

You should add some delay for updating the tooltip, otherwise it's changed with every written character.

@jsjtxietian
Copy link
Contributor Author

jsjtxietian commented Nov 10, 2023

You should add some delay for updating the tooltip, otherwise it's changed with every written character.

IMHO the best way is to update the tooltip when the user left the input box (when it loses focus).

Also how do we do this, use a timer, and when user input something new but the timer is not triggered, we reset the timer?

@jsjtxietian jsjtxietian force-pushed the update-scene-tree-tooltip-after-editor-description-chnage branch from d035392 to 269681c Compare November 10, 2023 05:02
@KoBeWi
Copy link
Member

KoBeWi commented Nov 10, 2023

Also how do we do this, use a timer, and when user input something new but the timer is not triggered, we reset the timer?

Yes, the timer should be restarted with each input.

@jsjtxietian jsjtxietian force-pushed the update-scene-tree-tooltip-after-editor-description-chnage branch 2 times, most recently from b938b00 to 2d1d92d Compare November 13, 2023 06:05
scene/main/node.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Nov 13, 2023

The timer should be in SceneTreeEditor, not Node.

@jsjtxietian
Copy link
Contributor Author

jsjtxietian commented Nov 13, 2023

Yes node is the wrong place (especiall when we dont have a editor macro) , but I wonder whether we should use a timer, seems a bit excessive

@jsjtxietian jsjtxietian force-pushed the update-scene-tree-tooltip-after-editor-description-chnage branch from 2d1d92d to ac7e452 Compare November 14, 2023 05:01
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The implementation looks alright overall, but I'd look into changing Timer to SceneTreeTimer. Though technically the PR works correctly and this can be done later.

@jsjtxietian jsjtxietian force-pushed the update-scene-tree-tooltip-after-editor-description-chnage branch from ac7e452 to 4680ced Compare November 15, 2023 06:28
@akien-mga akien-mga merged commit 73c5def into godotengine:master Jan 3, 2024
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
Development

Successfully merging this pull request may close these issues.

Tooltip from Editor Description field should propagate immediately
4 participants