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

runtime: Handle widget operations in program::State helper #1913

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

Drakulix
Copy link
Contributor

This handles widget operations in the State-helper. As far as I am aware the shell/window-integrations all use the UserInterface-type directly, while the State-helper was initially conceived for foreign integrations, like the previously existing integration_opengl and integration_wgpu examples.
It seems like it hasn't gotten a lot of attention recently, although it is quite convenient for smaller integrations. (I am using this type in cosmic-comp to render parts of the shell with iced.)

Given State is mostly a convenience type around the UserInterface building block and hides that behind it's api, so there is no way to call UserInterface::operate from the outside. Additionally it returns Commands created by the Widget-Tree unprocessed.
Any Actions::Widget-variants derived from these Commands containing Operations are thus never processed by the internal UserInterface and cannot be passed in from the outside.

This PR fixes this short-coming and thus makes the State-helper more useful for integrations. I opted to handle this inside the helper, given that the only sensible thing to do with a Command is to convert it into Actions anyway (from my limited understanding). Additionally this keeps the State api quite simple (as was intended?).
As such State::update now filters Actions for widget operations and sends those back into the UserInterface before returning the rest in a similar vain to the commands previously.

I know I have skipped reaching out or opening an RFC, but I have no problem with this PR being rejected, if another way to solve this is preferred. It was a relatively small and easy change and quick to prototype.
I hope to get some discussion on how to best fix this limitation started. I am happy to solve this a different, e.g. if it would be preferred to add a operate-function to the State-type instead or make it possible to access the internal Cache/UserInterface.
Alternatively State could be dropped if not deemed useful enough any more, but for strongly advocate for keeping it for simpler integrations. It is a pretty nice api apart from this limitation. :)

@13r0ck
Copy link
Member

13r0ck commented Jun 13, 2023

Note: The pop-os fork of this (pop-os#46) Should be reverted with whatever the resolution to this is

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

I think it'd make more sense to introduce an operate method to State. Did you consider this approach?

The current approach here doesn't properly leverage type-safety. The actions are filtered, but the user will still have to match on Action::Widget anyways.

@hecrj hecrj added feature New feature or request shell labels Jun 27, 2023
@Drakulix Drakulix force-pushed the feature/runtime-state-operate branch from dd34591 to 4b831a9 Compare June 28, 2023 16:51
@Drakulix
Copy link
Contributor Author

I think it'd make more sense to introduce an operate method to State. Did you consider this approach?

Thanks for the feedback. I did, but felt like the slim api of State, might have been intentional.

I changed the PR to add an operate method instead. Given this has to built the UserInterface to handle operations, I opted for accepting and processing multiple Operations in one go.

Is this more to your liking?

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Yup! That's better!

Thank you 🙇

@hecrj hecrj enabled auto-merge June 29, 2023 06:17
@hecrj hecrj merged commit 949eca3 into iced-rs:master Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants