pop-os / xdg-desktop-portal-cosmic

GNU General Public License v3.0
27 stars 22 forks source link

feat: screenshot-ui #13

Closed wash2 closed 6 months ago

ids1024 commented 6 months ago

Requiring PORTAL_WAYLAND_SOCKET seems a bit awkward for testing (it's handy to be able to simply cargo run xdg-desktop-portal-cosmic. And if needed set WAYLAND_DEBUG or run gdb).

Not sure how best we want to deal with that.

wash2 commented 6 months ago

Not sure how best we want to deal with that.

I guess we could avoid it if we added events and commands for all of the privileged protocols to iced-sctk :sweat_smile:

Drakulix commented 6 months ago

Not sure how best we want to deal with that.

I guess we could avoid it if we added events and commands for all of the privileged protocols to iced-sctk 😅

In theory we should be able to create multiple event queues from the same socket. Or use the security-context protocol to get new (also privileged) sockets like the portal.

All pretty ugly, but passing two sockets from cosmic-session is also somewhat ugly. (Though not really a problem.)

Anyway, all long term solutions. I guess for now this is good enough. Given cosmic-screencopy is privileged anyway, we need the cooperation with cosmic-session and running standalone isn't really possible anyway.

I guess we might need a --debug flag for cosmic-session, that then offers a dbus service to get privileged sockets for development at some point?

wash2 commented 6 months ago

While this implementation is missing a few things:

I think it is ready for review

ids1024 commented 6 months ago

For some reason I'm not seeing anything show up when I run cosmic-screenshot. It just blocks without doing anything.

wash2 commented 6 months ago

For some reason I'm not seeing anything show up when I run cosmic-screenshot. It just blocks without doing anything.

I assume you've updated cosmic-session? When I tested earlier with wgpu, it would hang, and also using the dbus-config feature caused problems until a recent commit. Are there any logs? I've been testing on hybrid graphics, but I'll try testing using nvidia and integrated too.

ids1024 commented 6 months ago

Yeah, it was just crashing until I updated cosmic-session. Now xdg-desktop-portal-cosmic doesn't seem to be logging anything. Seems odd.

I'm testing on Amd only (HP Dev One) at the moment.

wash2 commented 6 months ago

The missing logs is probably because the default log level for env_logger is set to error. I've noticed while testing that if i kill the portal and then quickly run cosmic-screenshot after, it hangs until I kill it again. If I wait for it to restart, it doesn't seem to hang. I'll have to look more into why.

ids1024 commented 6 months ago

if i kill the portal and then quickly run cosmic-screenshot after, it hangs until I kill it again.

Is it being socket activated by xdg-desktop-portal? It shouldn't since org.freedesktop.impl.portal.desktop.cosmic.service has Exec=false. Maybe xdg-desktop-portal decides xdg-desktop-portal-cosmic isn't available and skips over that backend.

wash2 commented 6 months ago

Maybe xdg-desktop-portal decides xdg-desktop-portal-cosmic isn't available and skips over that backend.

I think this is most likely what happened.

ids1024 commented 6 months ago

After updating everything and starting a new instance of cosmic-session, this seemed to be working quite well. But after running killall cosmic-session and starting it again, I see it not working.

I think when that happens, running killall xdg-desktop-portal fixes it?

But if the way we're using xdg-desktop-portal is a bit fragile, I don't think this PR itself has made it worse. Requiring xdg-desktop-portal-cosmic to have a privileged socket clashes a bit with how xdg-desktop-portal expects to be able to socket activate it.

wash2 commented 6 months ago

Requiring xdg-desktop-portal-cosmic to have a privileged socket clashes a bit with how xdg-desktop-portal expects to be able to socket activate it.

Ya :/

wash2 commented 6 months ago

But if the way we're using xdg-desktop-portal is a bit fragile, I don't think this PR itself has made it worse.

Should this PR be ok to merge then?

ids1024 commented 6 months ago

Window selection is broken if the windows don't fit side-by-side, since they don't scale down. But we can leave that for now and I'll try to add a widget to libcosmic that can handle that use case better for both screenshots and cosmic-workspaces (and see if Iced can get improved support for that sort of layout).

wash2 commented 6 months ago

Window selection is broken if the windows don't fit side-by-side, since they don't scale down.

Ahh, I see what you mean by that. I think I can fix that with Length::FillPortion(1)

ids1024 commented 6 months ago

https://docs.rs/iced/latest/iced/enum.Length.html#variant.FillPortion documents FillPortation(1) as equivalent to Fill.

That may work better, but is still not quite the right layout. The window preview is given the full available width, even if it doesn't need/use all of it:

DP-6

I think Iced may need some concept of the "natural" or preferred size of a widget, like GTK has. I'm looking into how this can be improved.

wash2 commented 6 months ago

Ok I see what you mean. I think I can probably fix this so at least the button isn't way too big by wrapping in a container.