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

Inherit widget name from parent #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jan200101
Copy link
Contributor

No description provided.

Copy link
Member

@Guldoman Guldoman left a comment

Choose a reason for hiding this comment

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

The issue with this approach is that widgets with a parent will never use their own name.

What do you think about either:

  1. Making it inherit the name from the parent in Widget:new, and returning that in get_name. This way if a widget sets a name, it will override the inherited one. Main drawback is that this won't follow changes to the parent's name.
  2. Setting it to nil in new, the using the parent's name in get_name, if self.name is still nil. This would allow setting a custom name, as well as following changes to the parent's name. Main drawback here is that things might not do a nil check when using Widget.name directly.

@Jan200101
Copy link
Contributor Author

This PR came from a PR I made to Pragtical which was merged as is.
Change it if you want, I don't care and its open to be edited by maintainers

@Jan200101
Copy link
Contributor Author

coming back to this
I don't think 1 is a good fit because if the parent changes its name it would never be inherited

2 sounds good tho

takase1121 pushed a commit that referenced this pull request Jul 29, 2024
This change gives a better chance of properly detecting if a font is
monospaced due to changes on pragtical/pragtical#122
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants