-
Notifications
You must be signed in to change notification settings - Fork 868
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
Handle _selected_range sent to NSTextInputClient.setMarkedText(). #3619
Handle _selected_range sent to NSTextInputClient.setMarkedText(). #3619
Conversation
Some CI typo checks might need improvements... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;).
Memo: I'm revising the sanity check of the get_actual_range_bounds(). |
The revision is done. I guess this PR is ready for merge. P.S.: I just don't know how to handle the |
It may be better to squash all the commits in this PR to one. |
b6ce256
to
ed8c901
Compare
95e3472
to
a2914dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r @madsmtm
Besides the @madsmtm review. You should pass the |
You should install it with |
@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.
|
@pan93412 I managed to install the nightly toolchain but still got this after running I can't understand why we have to waste time on unstable toolchains. |
04f041e
to
8c70dda
Compare
@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? |
To pick up the nightly toolchain, run Some Rust users prefer the |
@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.
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. |
The general rule is to separate paragraphs with a blank line. I also don't like |
If I were you, I'll clearly write these things in
What I experienced today is of extreme frustration. I got blamed by those users of my input method app 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.) |
@kchibisov |
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
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).
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.
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. |
(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... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I agree with Madsmtm's amendment. |
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. |
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. |
Thanks, and apologies that it took so long to get merged, I kept forgetting about it! |
changelog
module if knowledge of this change could be valuable to usersThis only solves the
_selected_range
part of this issue: #3617