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

set_ime_cursor_area uses the top left corner of the rectangle as the composition/cursor point on X11 #3965

Open
xorgy opened this issue Oct 23, 2024 · 6 comments · May be fixed by #3966
Open
Labels
B - bug Dang, that shouldn't have happened DS - x11

Comments

@xorgy
Copy link

xorgy commented Oct 23, 2024

Description

On X11, unlike other platforms (windows, macOS), Window::set_ime_cursor_area behaves differently from how it is described in the documentation.
In particular, it ignores the height of the rectangle, and submits the top left corner as the composition/insertion point:

#[inline]
pub fn set_ime_cursor_area(&self, spot: Position, _size: Size) {
let (x, y) = spot.to_physical::<i32>(self.scale_factor()).into();
let _ = self.ime_sender.lock().unwrap().send(ImeRequest::Position(
self.xwindow as ffi::Window,
x,
y,
));
}

Whereas on Windows, the height is added to produce the equivalent:

let composition_form = COMPOSITIONFORM {
dwStyle: CFS_POINT,
ptCurrentPos: POINT { x, y: y + height },
rcArea: rc_area,

And on Mac, NSView gives a rectangle for this operation, which the system will avoid placing the candidate window over (still assuming the bottom left corner, typically)

#[method(firstRectForCharacterRange:actualRange:)]
fn first_rect_for_character_range(
&self,
_range: NSRange,
_actual_range: *mut NSRange,
) -> NSRect {
trace_scope!("firstRectForCharacterRange:actualRange:");
let rect = NSRect::new(
self.ivars().ime_position.get(),
self.ivars().ime_size.get()
);
// Return value is expected to be in screen coordinates, so we need a conversion here
self.window()
.convertRectToScreen(self.convertRect_toView(rect, None))
}

This results in the candidate window being reliably placed overtop the preedit text, without platform-specific workarounds.

OS and window mananger

Xorg 21

Winit version

0.30.5

@xorgy xorgy added B - bug Dang, that shouldn't have happened DS - x11 labels Oct 23, 2024
@kchibisov
Copy link
Member

I think the docs mention that it's not implemented on X11? The issue is mostly that I don't think there's an API for that, winit could try to e.g. put the thing below what was submitted, but it could break.

@kchibisov
Copy link
Member

The APIs you see on other platforms require you to pass a rectangle to not obscure an area, so they are just different and compositor positions the window in a way to not obscure it.

@xorgy
Copy link
Author

xorgy commented Oct 23, 2024

@kchibisov The problem is that if you use this API as documented, you will obscure your text 100% of the time with the most common X11 IME (iBus). If you ignore the documentation and send only a Position at the baseline with no size, you get usable behavior most of the time, but then you are misusing the API on every other platform.

The API being called in the X11 backend as far as I'm aware is meant to be given the equivalent of ptCurrentPos on Windows, as most implementations rarely place the candidate window anywhere but below the given point, but I may be wrong about that.

@xorgy
Copy link
Author

xorgy commented Oct 23, 2024

Of course, you could also make a case for adding width to x in this case, because ibus on XIM will still obscure the line if it is near the bottom of the screen.

@kchibisov
Copy link
Member

kchibisov commented Oct 23, 2024

The thing is that none of that will happen with any of the other backend, though, it' x11 and i don't care that much.

@xorgy
Copy link
Author

xorgy commented Oct 25, 2024

Alright so I've talked with Fcitx @wengxt and he's willing to accept an attribute on the XIM IC, I'll see if I can confirm with Mr. Fujiwara about a name for the attribute for adding this to iBus as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B - bug Dang, that shouldn't have happened DS - x11
Development

Successfully merging a pull request may close this issue.

2 participants