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 Checkbox::set_text function. #1346

Merged
merged 2 commits into from
Oct 25, 2020

Conversation

luleyleo
Copy link
Collaborator

I need this to port Checkbox to Crochet and it seems useful enough to go into Druid directly.

@luleyleo luleyleo added the S-needs-review waits for review label Oct 25, 2020
@ForLoveOfCats
Copy link
Collaborator

Would the usage case in Crotchet still be covered by this if it was a builder fn with_label or a constructor from_label like is on Button?

@ForLoveOfCats
Copy link
Collaborator

ForLoveOfCats commented Oct 25, 2020

Ah nevermind, I see that this is about updating the text value
Edit: And the new fn already takes a Label as an arg, ignore me I'm being silly apparently

@luleyleo
Copy link
Collaborator Author

@ForLoveOfCats No, it wouldn't. I want to change the label without having to recreate the widget. You noticed that already

On adding setters in general to Druid:

I believe Druid would benefit from such functionality. Recreating widgets to change their properties mostly works for widgets without internal state, but even those then require being properly reinitialized with WidgetAddedand so forth. And for widgets with internal state, recreating them will even cause user-visible problems. And if we add setters to those widgets, it would seem inconsistent not to have them on cheap-to-recreate widgets.

@ForLoveOfCats
Copy link
Collaborator

I agree that mutating widgets in-tree is a reasonable thing to be doing.

As this is not actually being passed a Label widget but instead the LabelText perhaps it would be more accurately called set_label_text or set_text? Otherwise this looks good to me now that I've actually looked at the usage case.

@luleyleo
Copy link
Collaborator Author

@ForLoveOfCats Sounds good! I renamed it to set_text and also updated the doc comments.

@ForLoveOfCats
Copy link
Collaborator

Missed the changelog but then it looks good to me 👍

@luleyleo
Copy link
Collaborator Author

Oops, now it should be right!

@ForLoveOfCats ForLoveOfCats changed the title Add Checkbox::set_label function. Add Checkbox::set_text function. Oct 25, 2020
@ForLoveOfCats ForLoveOfCats removed the S-needs-review waits for review label Oct 25, 2020
@luleyleo luleyleo merged commit ecd0a24 into linebender:master Oct 25, 2020
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