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

add support for X input method #691

Merged
merged 3 commits into from Jul 25, 2017
Merged

add support for X input method #691

merged 3 commits into from Jul 25, 2017

Conversation

Determinant
Copy link
Contributor

Reapply the changes to the latest master (#658).

Copy link
Contributor

@jwilm jwilm left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for all of your work figuring out what needs to be done for IME support with XIM and implementing a PR. Also thanks for the patience on Glutin/winit updates.

I've commented on a couple of areas where some refactorings could improve maintainability. This PR can be merged once those are resolved.

src/main.rs Outdated
padding_y: py, ..} = *terminal.size_info();
let nspot_y = (py + (row + 1) as f32 * ch) as i16;
let nspot_x = (px + col as f32 * cw) as i16;
display.window().send_xim_spot(nspot_x, nspot_y);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move all of this logic into a method on Display like update_ime_position(&terminal)?

src/term/mod.rs Outdated
@@ -640,7 +640,7 @@ pub struct Term {
alt: bool,

/// The cursor
cursor: Cursor,
pub cursor: Cursor,
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than making cursor pub, I think we should add an accessor that returns a read-only ref. Something like

fn cursor(&self) -> &Cursor {}

src/window.rs Outdated
@@ -183,6 +183,23 @@ impl Window {
title: &str
) -> Result<Window> {
let event_loop = EventsLoop::new();

#[cfg(any(target_os = "linux", target_os = "freebsd",
Copy link
Contributor

Choose a reason for hiding this comment

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

This block should be moved into a method like platform_window_init so that there's not conditionally compiled code in the method body.

@jwilm
Copy link
Contributor

jwilm commented Jul 24, 2017

Thanks for the changes; waiting for CI

@jwilm jwilm merged commit 4c4c2f5 into alacritty:master Jul 25, 2017
src/window.rs Show resolved Hide resolved
chrisduerr pushed a commit to chrisduerr/alacritty that referenced this pull request Apr 14, 2018
src/main.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants