Skip to content

Conversation

@jneem
Copy link
Member

@jneem jneem commented Jul 19, 2022

There's one little to-do left: the parenting isn't right because the window-identifier business in ashpd isn't working right now because of wayland crate version mismatches.

return self.inner.wl_surface.borrow().invalidate_rect(rect);
}

fn open_file(&self, options: FileDialogOptions) -> Option<FileDialogToken> {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor question: do we need to add these to every surface or can we just use the already implemented data() method to access the implementations directly vs expanding the api surface of handle.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was kinda wondering that too, but it seemed better to just follow what all the other methods were doing... I'd be happy to change it, though

Copy link
Contributor

Choose a reason for hiding this comment

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

the PR is fine strictly speaking as is; since you're basically following the pattern every backend is doing.... its just a nit i have with druids internals. these methods shouldn't need to be nested as deeply into the backends as they are. a backend really should not care about opening file dialogs. i've covered the topic in detail in other issues.

looks like you're only using the idle handle. which is already exposed, and long term the wayland id. I'd try to keep the file stuff as lightly coupled as possible personally. but rather see functionality implemented than worry about architectural issues that impact every backend currently =)

}

fn open_file(&self, options: FileDialogOptions) -> Option<FileDialogToken> {
// FIXME: current ashpd has issues with wayland versions
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a link to the upstream issue we could put here as a reference?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this. Which is merged, but there's still an issue with asphd'd wayland-client version not matching ours (and depending on a pre-release, which is a bit of a pain). I think the sanest thing to do is to wait until version 0.3 of the wayland-client libraries.

Also, it looks like getting an identifier requires a round-trip to the compositor, and so the new function is async...

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.

3 participants