Handle _selected_range sent to NSTextInputClient.setMarkedText(). #3619

ShikiSuen · 2024-04-01T18:44:53Z

Tested on all platforms changed // Only macOS.
Added an entry to the changelog module if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality
Updated feature matrix, if new features were added or implemented

This only solves the _selected_range part of this issue: #3617

ShikiSuen · 2024-04-02T01:34:26Z

Some CI typo checks might need improvements...

madsmtm

Thanks, I've been thinking about using these values as well in the past, but ended up forgetting about it exactly because the UTF-16 to UTF-8 conversion was a bit tricky, and I was lazy ;).

src/platform_impl/macos/view.rs

ShikiSuen · 2024-04-03T11:38:33Z

Thanks to @pan93412 and @madsmtm 's suggestions. Most of these suggestions are applied on the GitHub webpage, hence the spammed commit history. I apologize if that brings any inconvenience.

ShikiSuen · 2024-04-03T12:09:55Z

Memo: I'm revising the sanity check of the get_actual_range_bounds().
Maybe Pan misunderstood my intention here. I need to add some more inline commments.

ShikiSuen · 2024-04-03T15:44:18Z

The revision is done. I guess this PR is ready for merge.
Or we can wait for @madsmtm's refactor commit if he is glad to provide one.

P.S.: I just don't know how to handle the Id<T> in Rust...

pan93412 · 2024-04-03T15:46:55Z

It may be better to squash all the commits in this PR to one.

ShikiSuen · 2024-04-03T15:56:42Z

@pan93412 Thanks. Squashed.

This PR needs to be force-pulled due to the force-pushed suqashed commit. cc @madsmtm

src/platform_impl/macos/view.rs

kchibisov

r @madsmtm

kchibisov · 2024-05-03T18:26:58Z

Besides the @madsmtm review. You should pass the CI (use nightly formatter).

ShikiSuen · 2024-05-04T05:59:13Z

@kchibisov

pan93412 · 2024-05-04T06:00:57Z

@kchibisov

You should install it with rustup toolchain install nightly: https://rust-lang.github.io/rustup/concepts/channels.html

ShikiSuen · 2024-05-04T06:02:32Z

@pan93412 Sry but seems too late. Something seems to be permanently crapped on my computer due to the installation of the nightly.

I dunno why depending on nightly CI utilities for stability-critical scenarios.

~/Repos/winit > cargo install rustfmt-nightly                                           05/04/2024 02:00:22 PM
    Updating crates.io index
  Downloaded rustfmt-nightly v1.4.21
  Downloaded 1 crate (519.2 KB) in 1.53s
error: binary `cargo-fmt` already exists in destination
binary `rustfmt` already exists in destination
Add --force to overwrite
~/Repos/winit > cargo +nightly fmt                                                      05/04/2024 02:00:28 PM
error: toolchain 'nightly-x86_64-apple-darwin' is not installed
~/Repos/winit > cargo install rustfmt-nightly --force                                   05/04/2024 02:00:36 PM
    Updating crates.io index
  Installing rustfmt-nightly v1.4.21
    Updating crates.io index
  Downloaded autocfg v1.3.0
  Downloaded byte-tools v0.3.1
  Downloaded block-padding v0.1.5
  Downloaded vec_map v0.8.2
  Downloaded fake-simd v0.1.2
  Downloaded opaque-debug v0.2.3
  Downloaded block-buffer v0.7.3
  Downloaded ansi_term v0.11.0
  Downloaded arrayvec v0.5.2
  Downloaded dirs-sys v0.3.7
  Downloaded dirs v2.0.2
  Downloaded digest v0.8.1
  Downloaded fastrand v2.1.0
  Downloaded humantime v1.3.0
  Downloaded lock_api v0.3.4
  Downloaded ena v0.14.2
  Downloaded generic-array v0.12.4
  Downloaded derive-new v0.5.9
  Downloaded rustc-ap-rustc_lexer v677.0.0
  Downloaded rustc-ap-rustc_feature v677.0.0
  Downloaded psm v0.1.21
  Downloaded semver v0.9.0
  Downloaded scoped-tls v1.0.1
  Downloaded textwrap v0.11.0
  Downloaded thiserror-impl v1.0.59
  Downloaded stacker v0.1.15
  Downloaded synstructure v0.12.6
  Downloaded unicode-xid v0.2.4
  Downloaded unicode-width v0.1.12
  Downloaded semver-parser v0.7.0
  Downloaded anyhow v1.0.82
  Downloaded env_logger v0.6.2
  Downloaded quick-error v1.2.3
  Downloaded maybe-uninit v2.0.0
  Downloaded parking_lot_core v0.6.3
  Downloaded quote v1.0.36
  Downloaded memmap v0.7.0
  Downloaded thiserror v1.0.59
  Downloaded tracing-attributes v0.1.27
  Downloaded proc-macro2 v1.0.81
  Downloaded ignore v0.4.22
  Downloaded typenum v1.17.0
  Downloaded structopt v0.3.26
  Downloaded toml v0.5.11
  Downloaded serde v1.0.200
  Downloaded itertools v0.8.2
  Downloaded unicode_categories v0.1.1
  Downloaded cc v1.0.96
  Downloaded rustc-ap-rustc_parse v677.0.0
  Downloaded unicode-normalization v0.1.23
  Downloaded serde_derive v1.0.200
  Downloaded serde_json v1.0.116
  Downloaded clap v2.34.0
  Downloaded rustc-rayon v0.3.2
  Downloaded syn v2.0.60
  Downloaded rustix v0.38.34
  Downloaded term v0.6.1
  Downloaded rustc-ap-rustc_target v677.0.0
  Downloaded rustc-ap-rustc_data_structures v677.0.0
  Downloaded rustc-ap-rustc_ast v677.0.0
  Downloaded rustc-ap-rustc_expand v677.0.0
  Downloaded rustfmt-config_proc_macro v0.2.0
  Downloaded rustc-workspace-hack v1.0.0
  Downloaded rustc-ap-rustc_span v677.0.0
  Downloaded rustc-ap-rustc_serialize v677.0.0
  Downloaded rustc-ap-rustc_macros v677.0.0
  Downloaded rustc-ap-rustc_index v677.0.0
  Downloaded rustc-ap-rustc_graphviz v677.0.0
  Downloaded rustc-ap-rustc_fs_util v677.0.0
  Downloaded rustc-ap-rustc_errors v677.0.0
  Downloaded rustc-ap-rustc_attr v677.0.0
  Downloaded rustc-ap-rustc_ast_pretty v677.0.0
  Downloaded rustc-ap-rustc_ast_passes v677.0.0
  Downloaded rustc-ap-rustc_arena v677.0.0
  Downloaded parking_lot_core v0.7.3
  Downloaded measureme v0.7.1
  Downloaded md-5 v0.8.0
  Downloaded libc v0.2.154
  Downloaded cargo_metadata v0.8.2
  Downloaded bstr v1.9.1
  Downloaded annotate-snippets v0.6.1
  Downloaded rustc-rayon-core v0.3.2
  Downloaded rustc-ap-rustc_session v677.0.0
  Downloaded smallvec v0.6.14
  Downloaded diff v0.1.13
  Downloaded crossbeam-utils v0.7.2
  Downloaded crossbeam-epoch v0.9.18
  Downloaded annotate-snippets v0.8.0
  Downloaded parking_lot v0.10.2
  Downloaded parking_lot v0.9.0
  Downloaded jobserver v0.1.31
  Downloaded globset v0.4.14
  Downloaded either v1.11.0
  Downloaded termize v0.1.1
  Downloaded termcolor v1.4.1
  Downloaded rustc_version v0.2.3
  Downloaded structopt-derive v0.4.18
  Downloaded strsim v0.8.0
  Downloaded sha-1 v0.8.2
  Downloaded crossbeam-deque v0.8.5
  Downloaded bytecount v0.6.8
  Downloaded 101 crates (5.1 MB) in 1.54s
   Compiling proc-macro2 v1.0.81
   Compiling unicode-ident v1.0.12
   Compiling libc v0.2.154
   Compiling serde v1.0.200
   Compiling syn v1.0.109
   Compiling semver-parser v0.7.0
   Compiling autocfg v1.3.0
   Compiling cfg-if v0.1.10
   Compiling crossbeam-utils v0.8.19
   Compiling cc v1.0.96
   Compiling maybe-uninit v2.0.0
   Compiling typenum v1.17.0
   Compiling scopeguard v1.2.0
   Compiling lazy_static v1.4.0
   Compiling byteorder v1.5.0
   Compiling smallvec v1.13.2
   Compiling log v0.4.21
   Compiling smallvec v0.6.14
error[E0554]: `#![feature]` may not be used on the stable release channel
  --> /Users/shikisuen/.cargo/registry/src/index.crates.io-6f17d22bba15001f/smallvec-1.13.2/src/lib.rs:96:37
   |
96 | #![cfg_attr(feature = "may_dangle", feature(dropck_eyepatch))]
   |                                     ^^^^^^^^^^^^^^^^^^^^^^^^

   Compiling lock_api v0.3.4
   Compiling hashbrown v0.12.3
error[E0557]: feature has been removed
  --> /Users/shikisuen/.cargo/registry/src/index.crates.io-6f17d22bba15001f/lock_api-0.3.4/src/lib.rs:91:42
   |
91 | #![cfg_attr(feature = "nightly", feature(const_fn))]
   |                                          ^^^^^^^^ feature has been removed
   |
   = note: split into finer-grained feature gates

For more information about this error, try `rustc --explain E0554`.
error: could not compile `smallvec` (lib) due to previous error
warning: build failed, waiting for other jobs to finish...
For more information about this error, try `rustc --explain E0557`.
error: could not compile `lock_api` (lib) due to previous error
error: failed to compile `rustfmt-nightly v1.4.21`, intermediate artifacts can be found at `/var/folders/10/6wq7rrd57qx334gglsl_cfs00000gq/T/cargo-installghlNPo`.
To reuse those artifacts with a future compilation, set the environment variable `CARGO_TARGET_DIR` to that path.
~/Repos/winit >                                                                         05/04/2024 02:00:45 PM

ShikiSuen · 2024-05-04T06:08:47Z

@pan93412 I managed to install the nightly toolchain but still got this after running cargo fmt.

I can't understand why we have to waste time on unstable toolchains.

ShikiSuen · 2024-05-04T06:32:26Z

@kchibisov The nightly rustfmt hinders the TODO to be in a standalone line. Is there a way to stop the nightly rustfmt from doing this stupid?

pan93412 · 2024-05-04T06:35:56Z

@pan93412 I managed to install the nightly toolchain but still got this after running cargo fmt.

I can't understand why we have to waste time on unstable toolchains.

To pick up the nightly toolchain, run cargo +nightly fmt. You can switch your default toolchain to nightly by running rustup default nightly.

Some Rust users prefer the nightly toolchain, as it contains features that significantly improve the DX or are critical for some purposes. I also use nightly for my personal projects.

kchibisov · 2024-05-04T08:43:03Z

@ShikiSuen please, mind your language, even though it seems like you've deleted/edited a bunch of replies, it doesn't mean that I haven't seen them. This is not the first time when you behave impulsive, which may be off putting for certain people.

Some Rust users prefer the nightly toolchain, as it contains features that significantly improve the DX or are critical for some purposes. I also use nightly for my personal projects.

Stable rustfmt can not do good formatting unfortunately and it's like that for a very long time. As you can see, we use nightly only for fmt, and not for anything else.

If you have major issues with formatting, you should politely ask that you can not do that, instead of exaggerating.

kchibisov · 2024-05-04T10:43:52Z

The nightly rustfmt hinders the TODO to be in a standalone line. Is there a way to stop the nightly rustfmt from doing this stupid?

The general rule is to separate paragraphs with a blank line. I also don't like TODO in code in general, but it's up to @madsmtm to complain about such things, since it's not my backend.

ShikiSuen · 2024-05-04T11:23:16Z

@kchibisov

If I were you, I'll clearly write these things in README.md prior to ask the contributor to use cargo-fmt:

Requirements about nightly toolchain and nightly cargo-fmt.
Detailed instructions regarding how to install and run them to help stable-only Rust users to stay on the right track.

What I experienced today is of extreme frustration.

I got blamed by those users of my input method app
... because of all IMKTextInput client apps written in Rust directly or indirectly using this package.
... And I come here trying to fix the problem here to stop being blamed by my users.

And what I have experienced here today is totally unfair to me.

I'm sorry for what you have said "impulsive", but I really need to take a break.

(You might have felt something else impulsive from my previous comments. That's all because I got blamed by something that is technically not my fault... Especially the buggy InputMethodKit in macOS.)

ShikiSuen · 2024-05-04T11:37:11Z

@kchibisov
I added the formatting policy to the README.md through this PR:
#3680

kchibisov · 2024-05-04T15:37:24Z

Detailed instructions regarding how to install and run them to help stable-only Rust users to stay on the right track.

The rust itself does educate on how to install the toolchains and we don't require a particular nightly, etc, it's just any nightly. Though, there's no section in CONTRIBUTING.md that nighly needed, indeed.

And what I have experienced here today is totally unfair to me.

The thing is that this PR is not delayed because of new formatting change or anything like that, it's delayed because I want @madsmtm to look into it, and he is busy recently, even though I pinged him here numerous times. If PR is ready I'd just handle conflicts myself and merged on my own which is what I always do anyway, when I don't like something. It's just given that you decided to rebase recently, I decided to acknowledge that you in general should make your PR green (if they issue is on you on indicate that you don't know what to do).

I got blamed by those users of my input method app... because of all IMKTextInput client apps written in Rust directly or indirectly using this package.... And I come here trying to fix the problem here to stop being blamed by my users.

If users are blaming you for such thing, I'd just ignore them, because you don't owe anything them, unless you provide a paid service for them. Then it's a bit a different matter. Like I understand that it could be hard if you're not experienced dealing with that, but you can not please everyone.

In this particular case you send a patch and then it's really not your problem anymore.

(You might have felt something else impulsive from my previous comments. That's all because I got blamed by something that is technically not my fault... Especially the buggy InputMethodKit in macOS.)

It's a matter of the words you're using and how you react to things during the review, and it's not a language barrier or anything, because you need some knowledge to speak like that. Unless you've learned English by only talking in gaming discords or something, idk.

But besides all of that, keep in mind that winit in the end of the day is an open source project and we do that in our free time. I know that @madsmtm is not MIA, if they were, I'd handle that PR myself. Which requires more time from me than I want, etc, because I just don't like macOS in general.

ShikiSuen · 2024-05-04T16:24:05Z

(Re-review requests sent, conforming to the contribution guidelines.)

kchibisov · 2024-05-04T16:26:01Z

(Re-review requests sent, conforming to the contribution guidelines.)

I mean, there's no need to ask for re-review given that review was not submitted in the first place...

madsmtm

I pushed a commit to simplify the implementation, the extra function wasn't really required especially when I changed the way we got the string (there was some extra round-tripping between &str and NSString that wasn't necessary). Additionally I removed the sanity check, the range is documented to be in bounds, and Foundation is just going to throw an exception if it wasn't (i.e. it's not unsound to pass a wrong index).

Thanks @ShikiSuen, and apologies I didn't get to this sooner.

ShikiSuen · 2024-05-05T16:33:56Z

I agree with Madsmtm's amendment.
Is there anything unfinished prior to merging this PR?

ShikiSuen · 2024-05-05T16:37:26Z

i.e. it's not unsound to pass a wrong index

Looks like it is just my lack of familiarity with Foundation working on programming languages other than Swift. In Swift it is always an asserted death if an out-of-bound index gets passed into an array.

kchibisov · 2024-05-05T16:43:23Z

Swift it is always an asserted death if an out-of-bound index gets passed into an array.

yet, it's still fine to do so in swift, it'll just assert the same way. unsound would mean that safe code invokes undefined behavior, panic is not unsound.

ShikiSuen · 2024-05-05T16:47:03Z

Swift it is always an asserted death if an out-of-bound index gets passed into an array.

yet, it's still fine to do so in swift, it'll just assert the same way. unsound would mean that safe code invokes undefined behavior, panic is not unsound.

Yes. What I want to say is that in such situation Swift doesn't throw an Exception for devs to handle. It just crashes instead. The generated IPS crash report in macOS console indicates that it is caused by array-out-of-bounds.

ShikiSuen requested a review from madsmtm as a code owner April 1, 2024 18:44

ShikiSuen mentioned this pull request Apr 1, 2024

Both _selected_range and _replacement_range parameters are ignored in NSTextInputClient implementation. #3617

Open

madsmtm requested changes Apr 2, 2024

View reviewed changes

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

pan93412 suggested changes Apr 3, 2024

View reviewed changes

ShikiSuen force-pushed the shiki/fix_set_marked_text branch from b6ce256 to ed8c901 Compare April 3, 2024 15:55

pan93412 reviewed Apr 5, 2024

View reviewed changes

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

ShikiSuen requested a review from madsmtm April 5, 2024 03:45

kchibisov requested changes Apr 9, 2024

View reviewed changes

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

ShikiSuen requested a review from kchibisov April 10, 2024 00:14

pan93412 suggested changes Apr 10, 2024

View reviewed changes

ShikiSuen force-pushed the shiki/fix_set_marked_text branch from 95e3472 to a2914dc Compare April 10, 2024 00:50

kchibisov approved these changes Apr 14, 2024

View reviewed changes

daxpedda added the DS - macos label Apr 18, 2024

pan93412 approved these changes May 1, 2024

View reviewed changes

ShikiSuen requested review from msiglreith and notgull as code owners May 1, 2024 16:47

Handle _selected_range sent to NSTextInputClient.setMarkedText().

8c70dda

ShikiSuen force-pushed the shiki/fix_set_marked_text branch from 04f041e to 8c70dda Compare May 4, 2024 06:28

Stop rustfmt nightly from hindering TODO to become a standalone line.

f9da61a

ShikiSuen requested review from kchibisov and pan93412 May 4, 2024 16:23

kchibisov approved these changes May 4, 2024

View reviewed changes

pan93412 approved these changes May 4, 2024

View reviewed changes

Simplify implementation

b5a5f88

madsmtm approved these changes May 5, 2024

View reviewed changes

madsmtm added 3 commits May 5, 2024 17:07

Fix typo

b3c1e86

Add changelog entry

14d5947

Fix formatting of changelog entry

dfc1732

Merge branch 'master' into shiki/fix_set_marked_text

780a5b2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle _selected_range sent to NSTextInputClient.setMarkedText(). #3619

Handle _selected_range sent to NSTextInputClient.setMarkedText(). #3619

ShikiSuen commented Apr 1, 2024

ShikiSuen commented Apr 2, 2024

madsmtm left a comment •

edited

ShikiSuen commented Apr 3, 2024 •

edited

ShikiSuen commented Apr 3, 2024

ShikiSuen commented Apr 3, 2024

pan93412 commented Apr 3, 2024

ShikiSuen commented Apr 3, 2024

kchibisov left a comment

kchibisov commented May 3, 2024

ShikiSuen commented May 4, 2024

pan93412 commented May 4, 2024 •

edited

ShikiSuen commented May 4, 2024

ShikiSuen commented May 4, 2024

ShikiSuen commented May 4, 2024

pan93412 commented May 4, 2024

kchibisov commented May 4, 2024

kchibisov commented May 4, 2024

ShikiSuen commented May 4, 2024 •

edited

ShikiSuen commented May 4, 2024

kchibisov commented May 4, 2024

ShikiSuen commented May 4, 2024

kchibisov commented May 4, 2024

madsmtm left a comment •

edited

ShikiSuen commented May 5, 2024

ShikiSuen commented May 5, 2024

kchibisov commented May 5, 2024

ShikiSuen commented May 5, 2024

Handle _selected_range sent to NSTextInputClient.setMarkedText(). #3619

Are you sure you want to change the base?

Handle _selected_range sent to NSTextInputClient.setMarkedText(). #3619

Conversation

ShikiSuen commented Apr 1, 2024

ShikiSuen commented Apr 2, 2024

madsmtm left a comment • edited

Choose a reason for hiding this comment

ShikiSuen commented Apr 3, 2024 • edited

ShikiSuen commented Apr 3, 2024

ShikiSuen commented Apr 3, 2024

pan93412 commented Apr 3, 2024

ShikiSuen commented Apr 3, 2024

kchibisov left a comment

Choose a reason for hiding this comment

kchibisov commented May 3, 2024

ShikiSuen commented May 4, 2024

pan93412 commented May 4, 2024 • edited

ShikiSuen commented May 4, 2024

ShikiSuen commented May 4, 2024

ShikiSuen commented May 4, 2024

pan93412 commented May 4, 2024

kchibisov commented May 4, 2024

kchibisov commented May 4, 2024

ShikiSuen commented May 4, 2024 • edited

ShikiSuen commented May 4, 2024

kchibisov commented May 4, 2024

ShikiSuen commented May 4, 2024

kchibisov commented May 4, 2024

madsmtm left a comment • edited

Choose a reason for hiding this comment

ShikiSuen commented May 5, 2024

ShikiSuen commented May 5, 2024

kchibisov commented May 5, 2024

ShikiSuen commented May 5, 2024

madsmtm left a comment •

edited

ShikiSuen commented Apr 3, 2024 •

edited

pan93412 commented May 4, 2024 •

edited

ShikiSuen commented May 4, 2024 •

edited

madsmtm left a comment •

edited