Skip to content

Add xx_input_method_v1 support. #1745

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

dcz-self
Copy link
Contributor

@dcz-self dcz-self commented May 23, 2025

Implements https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/407

Notably missing: repositioning, reactivity of the popup.

EDIT: this replaces the previous input method for now, so I made it a draft instead.

@dcz-self
Copy link
Contributor Author

To test:

# in anvil
cargo run --release -- --winit &
WAYLAND_DISPLAY=wayland-1 yad &
# in stiwri
WAYLAND_DISPLAY=wayland-1 cargo run --release --bin im_popup

@dcz-self
Copy link
Contributor Author

@dcz-self dcz-self changed the title Add xx_input_method_v1 support. DRAFT: Add xx_input_method_v1 support. May 23, 2025
@dcz-self
Copy link
Contributor Author

Lookng at CI failures, I need some help understanding what they are.

For example, this code wasn't even touched by my change.

  error: fields `popup_geometry_callback`, `new_popup`, and `popup_repositioned` are never read
     --> src/wayland/input_method/input_method_handle.rs:172:16

There's also the ack_configure callback which is for some reason exposed to the compositor, but I don't really understand why, so I couldn't come up with a sensible doc comment.

@ids1024
Copy link
Member

ids1024 commented May 23, 2025

For example, this code wasn't even touched by my change.

I think that's a result of the earlier warning. Because certain methods are never used, the variables that are read only in those methods are never read.

@dcz-self
Copy link
Contributor Author

Added repositioning:

WAYLAND_DISPLAY=wayland-1 cargo run --release --bin im_popup_reposition
Screencast_20250524_202116.webm

@dcz-self dcz-self force-pushed the im_popup branch 2 times, most recently from 335ccb6 to 9497a8f Compare May 25, 2025 13:25
@dcz-self
Copy link
Contributor Author

I did some cleanups:

  • lints, except the ack_configure which I don't understand
  • both input methods are supported and working

Is there anything I can do to make reviewing this easier?

@dcz-self dcz-self changed the title DRAFT: Add xx_input_method_v1 support. Add xx_input_method_v1 support. May 25, 2025
}
}

fn send_done(&self) {
match *self {
PopupKind::Xdg(ref t) => t.send_popup_done(),
PopupKind::InputMethod(_) => {} //Nothing to do the IME takes care of this itself
PopupKind::InputMethodV3(_) => {} //Nothing to do the IME takes care of this itself
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be useful as a way to communicate that the compositor closed the popup? Or is it redundant since normally that would follow from a deactivate?

Copy link
Contributor

Choose a reason for hiding this comment

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

There was nothing to do previously since there was no concept of how the popup should function. But you are free to add that now if it is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the only time the compositor closes the popup is in deactivate, so I decided there's no need for another notification. Would one be useful?

@rano-oss
Copy link
Contributor

run these on your branch and correct them btw:
cargo clippy --features "test_all_features" -- -D warnings
cargo fmt --all -- --check
cargo hack check --each-feature --no-dev-deps --exclude-features use_bindgen

@rano-oss
Copy link
Contributor

rano-oss commented Jun 11, 2025

LGTM otherwise, someone that knows xdg positioner should probably have a look at the positioner and popup part

@dcz-self
Copy link
Contributor Author

The only thing left that breaks ci is the ack_configure which I have no idea what to do with.

@rano-oss
Copy link
Contributor

Just add documentation to this function:

error: missing documentation for a method
   --> src/wayland/input_method_v3/mod.rs:131:5
    |
131 | /     fn popup_ack_configure(
132 | |         &mut self,
133 | |         _surface: &WlSurface,
134 | |         _serial: Serial,
135 | |         _client_state: PopupSurfaceState,
136 | |     ) {
    | |_____^
    |
    = note: `-D missing-docs` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(missing_docs)]`

@dcz-self
Copy link
Contributor Author

Well that's the problem, I don't have a clue what it's meant to do :P I copy-pasted it without understanding and I hope a reviewer can enlighten me.

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