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

adding added controller widget #1485

Merged
merged 12 commits into from
Jan 8, 2021
Merged

adding added controller widget #1485

merged 12 commits into from
Jan 8, 2021

Conversation

arthmis
Copy link
Collaborator

@arthmis arthmis commented Dec 24, 2020

I had an issue with the documentation display. I followed the Click widget as an example, but I don't think the documentation is showing up as expected. Also there seems to be extra ' in the Click widget documentation. Is there anything I missed in the implementation?

EDIT (Richard Dodd) Closes #1480.

@cmyr cmyr added the S-needs-review waits for review label Dec 28, 2020
Copy link
Collaborator

@jneem jneem left a comment

Choose a reason for hiding this comment

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

Looks good! I had some minor nitpicks with the docs.

druid/src/widget/added.rs Outdated Show resolved Hide resolved
druid/src/widget/added.rs Outdated Show resolved Hide resolved
}

impl<T: Data> Added<T> {
/// Create a new [`Controller`] widget to respond to widget added to tree event.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Splitting hairs, maybe, but a Controller isn't a widget (the ControllerHost is).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I was following the Click widget example. I don't remember if that's a full widget or a Controller type. So most of the docs I've written is paraphrasing Click's docs. I was trying to be consistent with that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know what would be preferred here. I'm leaving it as is. I think calling it a Controller widget is appropriate. Anything else might be too verbose. I guess you can just call it controller like, "Create a new controller to respond to widget added to tree event".

@jneem
Copy link
Collaborator

jneem commented Dec 31, 2020

Looks good to me! Unfortunately, clippy broke the build again and we can't merge until its fixed. I'll try to get to it later today if no one beats me to it.

@richard-uk1
Copy link
Collaborator

richard-uk1 commented Dec 31, 2020

I've fixed up clippy for the rust 1.49 in #1461. If you rebase on that after it's merged, the clippy warning should go away. :)

Update You can rebase now to get the clippy fixes. If you need any help rebasing ping me; I'm happy to help. 😄

@rjwittams rjwittams added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Jan 5, 2021
@arthmis
Copy link
Collaborator Author

arthmis commented Jan 6, 2021

Hi, I see the label waiting on author. Is there something I'm supposed to do?

@jneem
Copy link
Collaborator

jneem commented Jan 6, 2021

You'll need to rebase your branch onto the master branch, to pull in the fixes for the broken CI.

@arthmis
Copy link
Collaborator Author

arthmis commented Jan 6, 2021

Uhh did I rebase correctly?

@richard-uk1
Copy link
Collaborator

Rebasing is fiddly! I will check out the PR and fix it up when I get chance.

@richard-uk1 richard-uk1 removed the S-waiting-on-author waits for changes from the submitter label Jan 6, 2021
@cmyr
Copy link
Member

cmyr commented Jan 6, 2021

it looks like something went a bit wrong and you brought in a bunch of old commits. I would just do git fetch origin and git merge origin/master and then we can clean it up here, when we merge. :)

@arthmis
Copy link
Collaborator Author

arthmis commented Jan 6, 2021

Am i doing this from the current state?

Edit: Ok that should be right, now!

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Just skimming this again: I have a few little doc tweaks, but I also have one larger question, which is whether or not we should also be passing in the widget itself to the closure, as like &mut W? I think this probably makes sense.

druid/src/widget/widget_ext.rs Outdated Show resolved Hide resolved
druid/src/widget/added.rs Show resolved Hide resolved
druid/src/widget/added.rs Outdated Show resolved Hide resolved
druid/src/widget/widget_ext.rs Outdated Show resolved Hide resolved
@richard-uk1
Copy link
Collaborator

richard-uk1 commented Jan 6, 2021

@cmyr are we OK to merge now, and shall we squash? @arthmis thanks very much for going through the motions with this PR 😄

@cmyr
Copy link
Member

cmyr commented Jan 6, 2021

Yep squash sounds good :)

@richard-uk1 richard-uk1 merged commit 78e1570 into linebender:master Jan 8, 2021
@arthmis arthmis deleted the added branch January 8, 2021 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants