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

Jump to bottom

Revert "Add composition event on macOS (#1979)" #2119

Merged
merged 1 commit into from Jan 2, 2022
Merged

Revert "Add composition event on macOS (#1979)" #2119

Show file tree
Hide file tree
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
@@ -1,6 +1,7 @@
# Unreleased

- On X11, add mappings for numpad comma, numpad enter, numlock and pause.
- On macOS, fix Pinyin IME input by reverting a change that intended to improve IME.

# 0.26.0 (2021-12-01)

Expand Down
1 change: 0 additions & 1 deletion Cargo.toml
View file
Open in desktop
Expand Up @@ -48,7 +48,6 @@ cocoa = "0.24"
core-foundation = "0.9"
core-graphics = "0.22"
dispatch = "0.2.0"
block = "0.1"

[target.'cfg(target_os = "macos")'.dependencies.core-video-sys]
version = "0.1.4"
Expand Down
2 changes: 0 additions & 2 deletions src/platform_impl/macos/ffi.rs
View file
Open in desktop
Expand Up @@ -118,8 +118,6 @@ pub enum NSWindowLevel {
NSScreenSaverWindowLevel = kCGScreenSaverWindowLevelKey as _,
}

pub const NSStringEnumerationByComposedCharacterSequences: NSUInteger = 2;

pub type CGDisplayFadeInterval = f32;
pub type CGDisplayReservationInterval = f32;
pub type CGDisplayBlendFraction = f32;
Expand Down
40 changes: 3 additions & 37 deletions src/platform_impl/macos/util/mod.rs
View file
Open in desktop
Expand Up @@ -3,13 +3,8 @@ mod cursor;

pub use self::{cursor::*, r#async::*};

use std::{
cell::Cell,
ops::{BitAnd, Deref},
rc::Rc,
};
use std::ops::{BitAnd, Deref};

use block::ConcreteBlock;
use cocoa::{
appkit::{NSApp, NSWindowStyleMask},
base::{id, nil},
Expand All @@ -18,12 +13,8 @@ use cocoa::{
use core_graphics::display::CGDisplay;
use objc::runtime::{Class, Object, Sel, BOOL, YES};

use crate::{
dpi::LogicalPosition,
platform_impl::platform::ffi::{
self, NSRange, NSStringEnumerationByComposedCharacterSequences,
},
};
use crate::dpi::LogicalPosition;
use crate::platform_impl::platform::ffi;

// Replace with `!` once stable
#[derive(Debug)]
Expand Down Expand Up @@ -113,31 +104,6 @@ pub unsafe fn ns_string_id_ref(s: &str) -> IdRef {
IdRef::new(NSString::alloc(nil).init_str(s))
}

/// Returns the number of characters in a string.
/// (A single character may consist of multiple UTF-32 code units.
/// This is possible when long sequences of composing characters are present)
///
/// Unsafe because assumes that the `string` is an `NSString` object
pub unsafe fn ns_string_char_count(string: id) -> usize {
let length: NSUInteger = msg_send![string, length];
let range = NSRange {
location: 0,
length,
};
let char_count = Rc::new(Cell::new(0));
let block = {
let char_count = char_count.clone();
ConcreteBlock::new(move || char_count.set(char_count.get() + 1)).copy()
Copy link
Member

Choose a reason for hiding this comment

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

This was also UB btw, the block must take the four arguments as described in the docs (even if unused)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, I'll try to remember this for the future.

};
let block = &*block;
let () = msg_send![string,
enumerateSubstringsInRange:range
options:NSStringEnumerationByComposedCharacterSequences
usingBlock:block
];
char_count.get()
}

#[allow(dead_code)] // In case we want to use this function in the future
pub unsafe fn app_name() -> Option<id> {
let bundle: id = msg_send![class!(NSBundle), mainBundle];
Expand Down
108 changes: 27 additions & 81 deletions src/platform_impl/macos/view.rs
View file
Open in desktop
Expand Up @@ -29,7 +29,7 @@ use crate::{
scancode_to_keycode, EventWrapper,
},
ffi::*,
util::{self, ns_string_char_count, IdRef},
util::{self, IdRef},
window::get_window_id,
DEVICE_ID,
},
Expand Down Expand Up @@ -57,8 +57,6 @@ pub(super) struct ViewState {
raw_characters: Option<String>,
pub(super) modifiers: ModifiersState,
tracking_rect: Option<NSInteger>,
is_ime_activated: bool,
marked_text: id,
}

impl ViewState {
Expand All @@ -70,17 +68,13 @@ impl ViewState {
pub fn new_view(ns_window: id) -> (IdRef, Weak<Mutex<CursorState>>) {
let cursor_state = Default::default();
let cursor_access = Arc::downgrade(&cursor_state);
let marked_text =
unsafe { <id as NSMutableAttributedString>::init(NSMutableAttributedString::alloc(nil)) };
let state = ViewState {
ns_window,
cursor_state,
ime_spot: None,
raw_characters: None,
modifiers: Default::default(),
tracking_rect: None,
is_ime_activated: false,
marked_text,
};
unsafe {
// This is free'd in `dealloc`
Expand Down Expand Up @@ -153,10 +147,7 @@ lazy_static! {
sel!(setMarkedText:selectedRange:replacementRange:),
set_marked_text as extern "C" fn(&mut Object, Sel, id, NSRange, NSRange),
);
decl.add_method(
sel!(unmarkText),
unmark_text as extern "C" fn(&mut Object, Sel),
);
decl.add_method(sel!(unmarkText), unmark_text as extern "C" fn(&Object, Sel));
decl.add_method(
sel!(validAttributesForMarkedText),
valid_attributes_for_marked_text as extern "C" fn(&Object, Sel) -> id,
Expand All @@ -168,7 +159,7 @@ lazy_static! {
);
decl.add_method(
sel!(insertText:replacementRange:),
insert_text as extern "C" fn(&mut Object, Sel, id, NSRange),
insert_text as extern "C" fn(&Object, Sel, id, NSRange),
);
decl.add_method(
sel!(characterIndexForPoint:),
Expand Down Expand Up @@ -267,6 +258,7 @@ lazy_static! {
accepts_first_mouse as extern "C" fn(&Object, Sel, id) -> BOOL,
);
decl.add_ivar::<*mut c_void>("winitState");
decl.add_ivar::<id>("markedText");
let protocol = Protocol::get("NSTextInputClient").unwrap();
decl.add_protocol(&protocol);
ViewClass(decl.register())
Expand All @@ -276,9 +268,9 @@ lazy_static! {
extern "C" fn dealloc(this: &Object, _sel: Sel) {
unsafe {
let state: *mut c_void = *this.get_ivar("winitState");
let state = state as *mut ViewState;
let _: () = msg_send![(*state).marked_text, release];
Box::from_raw(state);
let marked_text: id = *this.get_ivar("markedText");
let _: () = msg_send![marked_text, release];
Box::from_raw(state as *mut ViewState);
}
}

Expand All @@ -287,6 +279,9 @@ extern "C" fn init_with_winit(this: &Object, _sel: Sel, state: *mut c_void) -> i
let this: id = msg_send![this, init];
if this != nil {
(*this).set_ivar("winitState", state);
let marked_text =
<id as NSMutableAttributedString>::init(NSMutableAttributedString::alloc(nil));
(*this).set_ivar("markedText", marked_text);
let _: () = msg_send![this, setPostsFrameChangedNotifications: YES];

let notification_center: &Object =
Expand Down Expand Up @@ -393,20 +388,17 @@ extern "C" fn reset_cursor_rects(this: &Object, _sel: Sel) {
extern "C" fn has_marked_text(this: &Object, _sel: Sel) -> BOOL {
unsafe {
trace!("Triggered `hasMarkedText`");
let state_ptr: *mut c_void = *this.get_ivar("winitState");
let state = &mut *(state_ptr as *mut ViewState);
let retval = (state.marked_text.length() > 0) as BOOL;
let marked_text: id = *this.get_ivar("markedText");
trace!("Completed `hasMarkedText`");
retval
(marked_text.length() > 0) as BOOL
}
}

extern "C" fn marked_range(this: &Object, _sel: Sel) -> NSRange {
unsafe {
trace!("Triggered `markedRange`");
let state_ptr: *mut c_void = *this.get_ivar("winitState");
let state = &mut *(state_ptr as *mut ViewState);
let length = state.marked_text.length();
let marked_text: id = *this.get_ivar("markedText");
let length = marked_text.length();
trace!("Completed `markedRange`");
if length > 0 {
NSRange::new(0, length - 1)
Expand All @@ -431,62 +423,32 @@ extern "C" fn set_marked_text(
) {
trace!("Triggered `setMarkedText`");
unsafe {
let state_ptr: *mut c_void = *this.get_ivar("winitState");
let state = &mut *(state_ptr as *mut ViewState);

// Delete previous marked text
let char_count = ns_string_char_count(state.marked_text.string());
delete_marked_text(state, char_count);

state.is_ime_activated = true;

let _: () = msg_send![state.marked_text, release];
state.marked_text = NSMutableAttributedString::alloc(nil);
let marked_text_ref: &mut id = this.get_mut_ivar("markedText");
let _: () = msg_send![(*marked_text_ref), release];
let marked_text = NSMutableAttributedString::alloc(nil);
let has_attr = msg_send![string, isKindOfClass: class!(NSAttributedString)];
if has_attr {
state.marked_text.initWithAttributedString(string);
marked_text.initWithAttributedString(string);
} else {
state.marked_text.initWithString(string);
marked_text.initWithString(string);
};

let text_ns_str = state.marked_text.string();
let slice = slice::from_raw_parts(
text_ns_str.UTF8String() as *const c_uchar,
text_ns_str.len(),
);
let text_str = str::from_utf8_unchecked(slice);

for character in text_str.chars() {
AppState::queue_event(EventWrapper::StaticEvent(Event::WindowEvent {
window_id: WindowId(get_window_id(state.ns_window)),
event: WindowEvent::ReceivedCharacter(character),
}));
}
*marked_text_ref = marked_text;
}
trace!("Completed `setMarkedText`");
}

extern "C" fn unmark_text(this: &mut Object, _sel: Sel) {
extern "C" fn unmark_text(this: &Object, _sel: Sel) {
trace!("Triggered `unmarkText`");
unsafe {
clear_marked_text(this);
let marked_text: id = *this.get_ivar("markedText");
let mutable_string = marked_text.mutableString();
let _: () = msg_send![mutable_string, setString:""];
Comment on lines +444 to +445
Copy link
Member

Choose a reason for hiding this comment

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

Not sure whether this or the other method (release + alloc) is better here, but let's just leave it be for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that releasing and allocating a new one would be safer, but the reference to markedText seems to be constrainted to the View, so it seems safe enough.

let input_context: id = msg_send![this, inputContext];
let _: () = msg_send![input_context, discardMarkedText];
}
trace!("Completed `unmarkText`");
}

/// Unsafe because assumes that `this` is an instance of the `WinitView` class that we declare
/// programmatically
unsafe fn clear_marked_text(this: &mut Object) {
let state_ptr: *mut c_void = *this.get_ivar("winitState");
let state = &mut *(state_ptr as *mut ViewState);

let _: () = msg_send![state.marked_text, release];
state.marked_text = NSMutableAttributedString::alloc(nil);

let input_context: id = msg_send![this, inputContext];
let _: () = msg_send![input_context, discardMarkedText];
}

extern "C" fn valid_attributes_for_marked_text(_this: &Object, _sel: Sel) -> id {
trace!("Triggered `validAttributesForMarkedText`");
trace!("Completed `validAttributesForMarkedText`");
Expand Down Expand Up @@ -534,19 +496,12 @@ extern "C" fn first_rect_for_character_range(
}
}

extern "C" fn insert_text(this: &mut Object, _sel: Sel, string: id, _replacement_range: NSRange) {
extern "C" fn insert_text(this: &Object, _sel: Sel, string: id, _replacement_range: NSRange) {
trace!("Triggered `insertText`");
unsafe {
let state_ptr: *mut c_void = *this.get_ivar("winitState");
let state = &mut *(state_ptr as *mut ViewState);

let is_ime_activated: bool = state.is_ime_activated;
if is_ime_activated {
clear_marked_text(this);
state.is_ime_activated = false;
return;
}

let has_attr = msg_send![string, isKindOfClass: class!(NSAttributedString)];
let characters = if has_attr {
// This is a *mut NSAttributedString
Expand Down Expand Up @@ -613,15 +568,6 @@ extern "C" fn do_command_by_selector(this: &Object, _sel: Sel, command: Sel) {
trace!("Completed `doCommandBySelector`");
}

fn delete_marked_text(state: &mut ViewState, count: usize) {
for _ in 0..count {
AppState::queue_event(EventWrapper::StaticEvent(Event::WindowEvent {
window_id: WindowId(get_window_id(state.ns_window)),
event: WindowEvent::ReceivedCharacter('\u{7f}'), // fire DELETE
}));
}
}

fn get_characters(event: id, ignore_modifiers: bool) -> String {
unsafe {
let characters: id = if ignore_modifiers {
Expand Down