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

Update iced #67

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Update iced #67

merged 1 commit into from
Nov 19, 2024

Conversation

git-f0x
Copy link
Contributor

@git-f0x git-f0x commented Oct 24, 2024

This currently panics, since it seems to enter unreachable code in the view function in app.rs (similar to pop-os/cosmic-greeter#149).

Edit: This now doesn't panic (enabling multi-window seems to fix it), but the OSDs show up as just a dot.

@ids1024
Copy link
Member

ids1024 commented Nov 6, 2024

Looking at the WAYLAND_DEBUG=1 logs, it shows a zwlr_layer_surface_v1#75.set_size(1, 1) call.

It seems size: None isn't working, and using something like size: Some((Some(300), Some(100))) makes the surface show as expected.

I guess that will need a fix in iced.

@ids1024
Copy link
Member

ids1024 commented Nov 6, 2024

Yeah, looks like the autosized layer-shell code that previously existed in iced_sctk (https://github.com/pop-os/iced/blob/061995084a5775b4fd7df63dda336be01ddf491c/sctk/src/application.rs#L2187-L2195) isn't implemented in the current iced_winit fork.

@git-f0x git-f0x marked this pull request as ready for review November 15, 2024 15:28
@git-f0x
Copy link
Contributor Author

git-f0x commented Nov 15, 2024

This seems to work well now. I've adjusted the design of OSDs with a value to match other similar widgets in Settings.
Also, should secure_input be used for the authentication dialog (for the password visibility toggle), instead of text_input?

@git-f0x git-f0x force-pushed the rebase branch 2 times, most recently from e6b5e4c to d606114 Compare November 15, 2024 18:54
@ids1024
Copy link
Member

ids1024 commented Nov 15, 2024

Good question. Perhaps it should use secure_input if echo is false, and just text_input otherwise.

The polkit dialog also seems to no longer focus the text box immediately on start as it did before, and requires manually clicking on the text box.

@git-f0x
Copy link
Contributor Author

git-f0x commented Nov 15, 2024

The polkit dialog also seems to no longer focus the text box immediately on start as it did before, and requires manually clicking on the text box.

That's also a reported issue after the App Library rebase, in that the search bar isn't focused initially. So it might be something that needs to be fixed in libcosmic/iced.
Edit: Though there's a PR there for potentially fixing that, so it can possibly also be fixed here.

@ids1024
Copy link
Member

ids1024 commented Nov 15, 2024

Looking at the documentation for the older and newer iced, it looks like Task::batch was changed to execute in parallel, when it was previously serial. So that change looks correct, and we'll want the same thing here.

@git-f0x
Copy link
Contributor Author

git-f0x commented Nov 15, 2024

I've been trying to do the same, but not quite sure where to chain the focus. Chaining it after cmd in the new method in polkit_dialog leads to borrowing issues. And I'm not seeing where else it could be placed.

@ids1024
Copy link
Member

ids1024 commented Nov 15, 2024

Hm, I guess it's not the same issue here, since cosmic-osd isn't using text_input::focus in a batch along with the get_layer_surface, but using it in LayerEvent::Focused and polkit_agent_helper::Event::Complete. That sounds like it should work...

@ids1024
Copy link
Member

ids1024 commented Nov 18, 2024

Looks like the ::Enter event just wasn't been sent, so this code wasn't reached.

pop-os/iced#195.

@git-f0x
Copy link
Contributor Author

git-f0x commented Nov 18, 2024

Yay!
Before this gets potentially merged (after libcosmic is updated), is the Makefile change ok, or should it be reverted?
I did that since it was slightly annoying to test it and revert to the repo version, with it being in /usr/local (and other repos don't put the compiled binary in local).

@ids1024
Copy link
Member

ids1024 commented Nov 19, 2024

I guess defaulting to /usr/local is conventional in most build systems? But yeah, it makes sense to match other Cosmic components.

@git-f0x
Copy link
Contributor Author

git-f0x commented Nov 19, 2024

The polkit dialog text input is now focused on launch!
This then seems to be fully functional (compared to current master).

Copy link
Member

@ids1024 ids1024 left a comment

Choose a reason for hiding this comment

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

Yep, seems good now. Thanks!

@ids1024 ids1024 merged commit 7eedda7 into pop-os:master Nov 19, 2024
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