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.

Sign up for GitHub

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

Fix repeated key input issue in Chrome with multiple windows #1642

Merged
merged 1 commit into from
Jan 23, 2025
Merged

Fix repeated key input issue in Chrome with multiple windows #1642

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/wayland/text_input/text_input_handle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::sync::{Arc, Mutex};

use tracing::debug;
use wayland_protocols::wp::text_input::zv3::server::zwp_text_input_v3::{self, ZwpTextInputV3};
use wayland_server::backend::ClientId;
use wayland_server::backend::{ClientId, ObjectId};
use wayland_server::{protocol::wl_surface::WlSurface, Dispatch, Resource};

use crate::input::SeatHandler;
Expand All @@ -21,20 +21,24 @@ struct Instance {
pub(crate) struct TextInput {
instances: Vec<Instance>,
focus: Option<WlSurface>,
enabled_resource_id: Option<ObjectId>,
}

impl TextInput {
fn with_focused_text_input<F>(&mut self, mut f: F)
where
F: FnMut(&ZwpTextInputV3, &WlSurface, u32),
{
if let Some(ref surface) = self.focus {
if let (Some(surface), Some(enabled_resource_id)) = (&self.focus, &self.enabled_resource_id) {
if !surface.alive() {
return;
}

for ti in self.instances.iter_mut() {
if ti.instance.id().same_client_as(&surface.id()) {
let instance_id = ti.instance.id();
if instance_id.same_client_as(&surface.id()) && instance_id.eq(enabled_resource_id) {
f(&ti.instance, surface, ti.serial);
break;
}
}
}
Expand Down Expand Up @@ -77,6 +81,13 @@ impl TextInputHandle {
self.inner.lock().unwrap().focus = surface;
}

fn set_enabled_resource_id(&self, resource_id: Option<ObjectId>) {
let mut inner = self.inner.lock().unwrap();
if inner.enabled_resource_id.is_some() != resource_id.is_some() {
inner.enabled_resource_id = resource_id;
}
}

Comment on lines +84 to +90
Copy link
Member

Choose a reason for hiding this comment

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

It works the other way around, the IME should state that it's ready meaning that it'll send the Entered to the client and it'll react with enabled and only one of them must be honored IIRC, however with this check it won't ever advertise since it won't be enabled until you actually call enabled, which means with this PR it won't ever happen, thus it will never work at all, thus not sure how it could ever worked, until the client just enables it because it can.

Copy link
Contributor Author

@hayandev hayandev Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry, you are right. I was focused on the behavior of enable and the input bug related to Chrome. I tested it extensively with Chrome and once with Qt and Gtk apps, and it worked well, so I thought it was working correctly (as shown in the attached image).

image

I just tested it several times with Gtk apps, and while it works in some cases, it fails more often, and as you mentioned, it's unclear why it works at all.

I would appreciate it if you could revert the commit. I apologize again for not testing more thoroughly.

Copy link
Member

Choose a reason for hiding this comment

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

I'll fix myself, don't worry.

Copy link
Member

Choose a reason for hiding this comment

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

Qt doesn't support text-input v3, so it worked just via dbus, I guess, and maybe some gtk apps worked that way as well, on a pure text-input-v3 system it couldn't possible work, just for the reference.

/// Send `leave` on the text-input instance for the currently focused
/// surface.
pub fn leave(&self) {
Expand Down Expand Up @@ -182,9 +193,11 @@ where

match request {
zwp_text_input_v3::Request::Enable => {
data.input_method_handle.activate_input_method(state, &focus)
data.handle.set_enabled_resource_id(Some(resource.id()));
data.input_method_handle.activate_input_method(state, &focus);
}
zwp_text_input_v3::Request::Disable => {
data.handle.set_enabled_resource_id(None);
data.input_method_handle.deactivate_input_method(state, false);
}
zwp_text_input_v3::Request::SetSurroundingText { text, cursor, anchor } => {
Expand Down
Loading