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

Clarify CanvasItem's visibility signal descriptions (followup) #97367

Conversation

AdriaandeJongh
Copy link
Contributor

@AdriaandeJongh AdriaandeJongh commented Sep 23, 2024

Follow-up to: #97223 (comment)

Simplifies the signal descriptions of CanvasItem's hidden and visibility_changed. I tried a couple different ways of having the description for hidden refer to the visibility_changed description, but as hidden is just above it, it would read weird? idk.

It's curious that the hidden signal still exists even though the visibility_changed signal (albeit together with a simple check to is_visible_in_tree in the connected callable) essentially supersedes it, but I don't know any C++ to deprecate the signal and start a transition to remove it. Perhaps someone else could pick that up.

@AThousandShips AThousandShips added enhancement documentation cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Sep 23, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Sep 23, 2024
@AdriaandeJongh AdriaandeJongh force-pushed the clarify-canvasitem-visibility-followup branch from a02a910 to c0a55da Compare September 23, 2024 16:06
@Mickeon
Copy link
Contributor

Mickeon commented Sep 23, 2024

but as hidden is just above it, it would read weird?

Personally, you usually need to look at each description such that they can be understood individually. Unless, say, there's a whole lot of explaining to do that does not warrant repetition.
Imagine the description as if it's showing up as a tooltip.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

I actually like the prior descriptions more, I feel out of the loop here. I do understand that top_level's existence makes the statement misleading, though.

@Mickeon
Copy link
Contributor

Mickeon commented Sep 23, 2024

It's curious that the hidden signal still exists even though the visibility_changed signal (albeit together with a simple check to is_visible_in_tree in the connected callable) essentially supersedes it, but I don't know any C++ to deprecate the signal and start a transition to remove

The hidden signal actually has a lot of applications internally, and I can assure you there will be a lot of people missing it if it were gone. Control nodes benefit from it the most, and it was especially useful back when Popup also used to inherit Control.
If anything, the oddity is that there's no corresponding shown signal. But this talk is beyond the scope of this PR.

@AdriaandeJongh
Copy link
Contributor Author

I actually like the prior descriptions more, I feel out of the loop here. I do understand that top_level's existence makes the statement misleading, though.

Ah, dang. I made the changes quickly as I share @kleonc's view to keep the description correct and compact, but I can see how it makes it less easy to comprehend. How do you suggest I continue @Mickeon?

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Sorry for a bit of delayed answer (if that was a concern).

The description can be compact, sure, but it also needs to be as meaningful and "digestible" as it can be with a few words. The previous description was too compact and didn't say anything much of substance, either.

@AdriaandeJongh AdriaandeJongh force-pushed the clarify-canvasitem-visibility-followup branch from c0a55da to 56ec624 Compare September 23, 2024 18:17
@AdriaandeJongh
Copy link
Contributor Author

It's tricky navigating the various different documentation styles of different collaborators, but I think my current wording is solid! Let me know what you think.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Just one more thing and I think this is excellent in my eyes.
Edit: Forgot to send.

@AdriaandeJongh AdriaandeJongh force-pushed the clarify-canvasitem-visibility-followup branch 2 times, most recently from f80f0b6 to 6865df3 Compare September 23, 2024 18:39
@Mickeon Mickeon modified the milestones: 4.x, 4.4 Sep 23, 2024
@AdriaandeJongh AdriaandeJongh force-pushed the clarify-canvasitem-visibility-followup branch from 6865df3 to b336918 Compare September 23, 2024 20:01
Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

Added last small nitpick, otherwise it's fine to me. Thanks @AdriaandeJongh for the patience with us (me)! 🙂

@AdriaandeJongh AdriaandeJongh force-pushed the clarify-canvasitem-visibility-followup branch from b336918 to 9b474e3 Compare September 24, 2024 06:31
@akien-mga akien-mga merged commit 9355845 into godotengine:master Sep 24, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@AdriaandeJongh AdriaandeJongh deleted the clarify-canvasitem-visibility-followup branch September 24, 2024 11:19
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.

5 participants