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

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

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

hayandev
Copy link
Contributor

@hayandev hayandev commented Jan 22, 2025

This PR fixes issue #1604

According to the text input v3 specification, the enable request should activate at most one text input on the compositor and ignore any subsequent enable requests until a disable request is received.

However, currently, Smithay does not guarantee this behavior and sends events to all text input objects created by the same client.

We will modify it to send events only to the text input object that has received the enable request and update the management of the enabled text input objects.

@hayandev hayandev force-pushed the fix-double-input-chrome branch from 19b4506 to a14b5d7 Compare January 23, 2025 00:33
Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for fixing this.
Found one minor nitpick.

src/wayland/text_input/text_input_handle.rs Outdated Show resolved Hide resolved
@hayandev hayandev force-pushed the fix-double-input-chrome branch from a14b5d7 to ef0d0e8 Compare January 23, 2025 15:45
@hayandev hayandev requested a review from Drakulix January 23, 2025 15:47
@Drakulix Drakulix merged commit 475072d into Smithay:master Jan 23, 2025
13 checks passed
@Drakulix
Copy link
Member

thanks!

@kchibisov
Copy link
Member

kchibisov commented Jan 24, 2025

After this commit fcitx text_input is straight doesn't work at all, juts gets instantly disabled and that's about it, to repro just run fcitx5 and try to run in gtk/alacritty/whatever all dead and not work.

Comment on lines +84 to +90
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;
}
}

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

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.

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