-
Notifications
You must be signed in to change notification settings - Fork 318
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
Text Input Method Editor #541
Conversation
Except for the tests failing, and this is something I forgot in my product prototype, too, the IME selection range should be canceled when the value is changed. The library user should enforce a new range if they still require it. |
This is what the visual feedback looks like: Windows usually uses a dashed or squiggly underline: Such lines, however, may be harder to replicate without generating too many vertices or modifying shaders. A solid line is, therefore, the best we can get right now. And any visual feedback is better than none. |
This looks very cool, and I agree that enabling users to properly do IME is important. I do have some questions and comments. I haven't looked at the code in any detail yet, I think it's useful to clarify some overall directions and principles first.
|
demo_61xjpGUUv1.mp4eurotrucks2_WMCMJvH4zl.mp4
Back to the second point. If we go by Unreal Engine, such input method context must do the following:
There are more methods and specifications. However, some are unimportant in this context, while others do not seem to be used. Again, I work on a custom game engine, and our input method context is a bit more trivial (based just on the points above). Now, let's take a look at the implementation details:
String value = control->GetValue();
start = ConvertCharacterOffsetToByteOffset( value, start );
end = ConvertCharacterOffsetToByteOffset( value, end );
value.replace( start, end - start, composition.data(), composition.length() );
int max_length = control->GetAttribute< int >( "maxlength", 0 );
int max_byte = ConvertCharacterOffsetToByteOffset( value, max_length );
if( value.length() > max_byte )
{
value = value.substr( 0, max_byte );
}
control->SetValue( value );
Please do not take me wrong; I am not trying to say that my proposal is and should be the final solution. I instead wanted to start a discussion about completing options for a custom IME implementation. And only the visuals seemed to be missing. If you spot any flaws in this or if I missed something, please let me know. |
First of all - this looks ridiculously good! Your video alone is a very good selling point. And thanks for the detailed explanation, this sounds very good to me. And I see that the API is quite simple, any IME/language-details would reside on the user-side, good! It would be extremely valuable if we could implement this feature on the Win32 backend. That could give us some feedback on the API, and would also serve as an example for other users to implement it on other platforms. Interface-wise, I wonder if it can be tricky to get all the calls correct in terms of selection/caret position and so on. It seems to be quite a lot of steps, and possibly a bit error prone. And I wonder if these steps would be similar regardless of backend. Maybe there is some way to simplify the usage by something like an IME-object that users could interact with. Or maybe keep it in the InputWidget, but make the API somehow deal with some of these complexities. I don't know exactly how that would look like, but I believe having a backend implementation could help us guide this. |
Thanks for the kind words. It is refreshing to see such an enthusiastic library author. :) Implementing "real" IME is tricky, especially when trying to do this "properly" via the Text Services Framework. In my case, I have been only a subscriber to an existing backend system implementing an editor handling predefined events. This is much easier, and I can focus purely on the front end (which is not trivial, either). Here, I cannot imagine the complexity, considering all platforms and possible text input methods (Hangul, Pinyin, Quick, Bopomofo, Kana-Kanji, etc.) with 100% support. Realistically speaking, the Unreal Egine's implementation is probably the best where the default candidate window is shown, and the engine handles the composition string. But again, the system has 1,000+ lines of code and is only for Windows. On the other hand, this is a game engine handling more than just a user interface. Regarding the interface, I am unsure what length the library should go. Should it be a complete system or only a set of editing functions with events? Even if we go only for the latter, I do not think I am the right person to draft any interface. Even though I have spent a lot of time on it, I am far from understanding the whole picture. But to give some examples from the real world, Coherent Gameface, a game user interface library, seems to encapsulate their IME implementation and expose only a very few setters and events: https://docs.coherent-labs.com/cpp-gameface/integration/optional_features/ime_native/ However, I am not really sure how it works with all the text input methods, and it does not provide much customizability for the user of the library either. When I take a look at my To address your worries, the usage is straightforward, actually, as shown in my previous comment (some implementations are really just one-liners). The only catch is that one needs to access the focused text input element. This is why I opened #540. In conclusion, with this pull request, I believe the library does only what it should do. Perhaps we could add styling for the IME feedback, although, as I mentioned earlier, rendering then gets tricky. Or we can add more methods to text input elements with proper documentation and guides to make the connection between RmlUi and existing game engines (in-house, Unreal Engine, etc.) more accessible. I will leave this up to you. But I personally want to avoid bloating the library with an improper IME solution. |
Appreciate the kind words! To me, it seems the best course of action for RmlUi is mainly to provide an interface to enable the user to glue our text widgets with what a proper IME library/interface can provide. We should try to keep the complexity low. Something like what coherent provides looks very reasonable. Unfortunately, those Unreal Engine-links don't work for me, I believe one needs to be a subscriber. It would be very hard to design and maintain such an interface without any sort of example to test it out with. That's why I believe, if we do make such an interface, we should have at least a minimum viable product in one of the backends. If we're able to put some of the complexity in the backend, I would not consider that bloat, as that would be completely opt-in from the user perspective, hundreds of lines of code here sounds fine. It's very hard for me to consider an interface without such an example, and especially since I don't really know what UE or other engines require. This PR itself I believe is a good step. I wonder if we should be a bit more generic in terms of naming, and call it "highlight" instead? In the future, we might want to implement a way to style this highlight using RCSS. But let's not implement styling for now, keep it simple to begin with. |
I apologize for not getting back to you sooner. My work and real life have caught up with me. I have followed the simplest solution, which this pull request initially proposed. Essentially, we are only adding the missing method for visual feedback. This is enough to create a minimum viable example (1a060ce) of implementing an interface of a game engine, as I explained in one of my comments above. In the example, I do quite a bit of work with IME. However, I have limited experience with it, so I cannot tell if anything could be improved (even though, most of the cases I have tested work just fine). It would be lovely if someone else could take a look and share their knowledge. Maybe some bits of it could be moved to the backend itself, which would work with the general interface of a text input method editor implemented by the user. The Regarding the method's name, I wonder if we should limit it to highlighting or provide a more versatile usage in the future, especially if we decide to move some of the IME stuff to the library itself. If not, would you be okay with By the way, I included Noto Sans in the sample project to display all character sets correctly, especially since IME is for languages such as Chinese, Japanese, or Korean, and barely any other font covers all of these character sets. This font is a bit heavier, so I hope it is fine with you. |
I finally got around to looking at this, and I've played quite a bit with it now. It's very cool, I really like it! You have me very much convinced that this is something we could support from the library side without any particular concerns. I tested it with Japanese input method and the Emoji panel. I also don't have any experience with using CJK or IMEs, but it seems to work well from what I can tell. One minor difference I noted when comparing to web browsers: In the sample, when entering an emoji from the panel, it replaces the text but doesn't end the composition range. Another small thing, when we do our own IME rendering, we should position the IME box at the bottom of the text, thereby adding the I think it could make sense to include this directly in the Win32 backend. It already works best with that backend, because we're positioning the IME panel only from here. The I quite like the term "composition range" you used in the sample, maybe we could name the function As for including the fonts. I was going to say "sure" until I saw how big they were. These fonts alone increase the zipped version of the library source from 4 MB to 15 MB :O If we want to add more variations or scripts later on, especially now that we started testing support for harfbuzz and arabic, this doesn't feel right. I wonder if we could make a CMake script that downloads any desired languages as needed? Maybe this IME sample is something you'd be interested to look at @xland? It would be very helpful with feedback and testing from someone more experienced with these subjects. |
Thank you for all the feedback. I appreciate you taking the time to review this pull request thoroughly. I cannot replicate the issue with emojis. The only thing I have noticed is the cursor moving away from the actually inputted text, which is caused by not converting the character bytes received from the Windows message to an actual character count. When it comes to using I agree with implementing a separate interface, Regarding the font, I could instead use unifont that covers all character sets. However, that will still at least double the current repository size. Otherwise, I do not know how to show the user the potential of this sample. As with CMake, this is rather difficult; the fonts come from Google Fonts, which would not work well with download scripts. |
e9a4d22
to
b08f293
Compare
As we discussed, I have implemented IME straight into the library based on the comments above and the previous sample: 609065b. This is currently implemented only for the Windows platform. Moreover, the sample now uses GNU Unifont, a lightweight solution compared to Noto Sans: ff5df43. The zipped version of the library is now 5.3 MB. I do not think that is too bad, but if you have any ideas for improvements, they are more than welcome. I have also fixed the bug with the cursor position mentioned in my latest comment. This was done by calculating the character position as the length of the UTF-8 substring from the WTF-16 composition string up to the RmlUi/Backends/RmlUi_Platform_Win32.cpp Lines 328 to 336 in a7af17a
This is some dark magic, but that goes for the entire Imm API and WTF-16. :) A new important addition is Anyway, I am still confused about what you wanted to do with |
83984ea
to
e022074
Compare
Very cool, thanks a lot! I'm wrapping up some other things right now, I'll take a closer look at this soon :) |
Hi again. I took a closer look at this one now, thanks for the patience. First of all, I think it's really cool! I'm generally happy with the architecture, and it seems to work well for me. While I like that GNU Unifont is quite a bit lighter, it seems to be not on par in terms of quality. And it's still a bit on the heavy side. Instead, considering that the implementation currently only works on Windows, I propose that we use system fonts instead. I played around with this a bit, and here is the proposal I came up with: d353a5f It seems to be simple enough, and works for me at least. Here is a comparison: To me, it looks like the quality is quite a bit better. And we also get to show off our color emoji support :) Some other, various comments:
You may need some changes after rebasing on top of master, since we merged the effects branch. Hopefully, not too much. Let me know when you are ready for a more detailed review by marking the pull request as ready for review. |
e022074
to
8e4b79b
Compare
Hey, thank you very much for your feedback and for looking into all of this. The idea of using system fonts is brilliant. I have never realized that loading them is as simple as this. I agree that this is a much better solution than relying on a third-party font, especially as this sample is available only on Windows. To answer your comments:
Rebasing was not very complicated. The only issue is the macOS build is failing for some reason. I would need to look into this. |
This branch does not cause the problem with the macOS build. When I compared the build log from this branch with the master, I noticed that the version of macOS and the FreeType library path were different. Have you made any changes to the settings of this repository? Or, possibly, some external changes caused the build to fail. |
Nice, thanks for following up on this! Glad to hear the rebase went smoothly. I'll get back to you with the review. I think it's perfectly fine to have this as a separate sample to begin with. I may take a look at integrating the functionality with all the samples at a later stage. Great that you only enable it with the Win32 backend now. Harfbuzz essentially makes text look a lot better, especially for some non-Latin languages. See the screenshots in #568 for some examples. Feel free to experiment with that, but not necessary for a first PR. Looks like Github updated the macOS image, which caused our library to not build correctly. You can simply ignore that here, or rebase on master again where I removed it. Looks like our new |
cf21e06
to
a7c6685
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.
I took a closer look at this one finally, apologies for leaving you waiting for a while. Now I should have some more time to follow up on these things, having merged the cmake rewrite. This will require some CMake changes if you rebase again (promise this is the last one for a while!). But if you prefer, I can take care of that in the end.
First of all, I really like how it looks. It works well for me, and the implementation looks good.
There are a couple of things about the overall architecture and the interfaces (TextInputMethodEditor
in particular) that I believe we could improve.
- One being that it seems to over-generalize. There are a lot of virtual functions that seems to act more like implementation details, at least considering the default implementation.
- Second being that it acts both as an interface to be used by the backend, and also from the element itself. It would be nice if we can find a better way to separate these usages.
I think more generally, we can reduce the size of the interfaces here. Reducing the number of virtual functions should make it easier to use and understand. Further, the less we put in public interfaces, the more room we have for future refactoring and adapting the implementation to new needs.
- I actually think we can replace the
TextInputMethodEditor
with the default implementation, and make the functions non-virtual. - Many of the public functions could be moved to the private section.
- Consider moving the
TextInputMethodContext
class to a private header. - The
TextInputMethodEditor
class might be better placed in Rml::Context instead of the system interface, and then the user can get it from there. If we see that we are left with relatively few functions in the IME editor, we could consider adding them directly to the context.
The implementation details themselves look good to me, I just think we can move things around a bit to improve the architecture. Please see some of my in-code comments as well.
Oh yes, looks like we have come fill circle indeed! Sounds good, I like the suggested |
Incorporated requested changes in the pull request: * Rename TextInputMethodContext to TextInputContext. * Make TextInputMethodEditor backend-specific. * Make the TextInputMethodEditor behavior well-defined instead of relying on assertions. * Add TextInputHandler (reimplementation of mikke89#548) as a global replacement (context-dependent). * Retrieve the text input's bounding box via an element utility function.
Conflicts: Samples/readme.md
First of all, I apologize for the delay. My work got busy, and I attended the Game Access Conference last week. Therefore, I also hope that most of this work still makes sense; it was done before I got caught up with other projects. Most of the changes are covered in a269d23:
Except for the possible addition of the two methods for |
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.
Great stuff! First of all, no worries, hope you had a nice trip. I am happy to see new progress, and really like how this is coming together. It looks quite a bit tidier and overall makes more sense to me now, especially with the IME editor / backend changes.
I have a few comments, most of them are relatively minor, but a few design points remain.
I've been playing a bit more with IME enabled. One issue I found: If you start writing something with the IME open, then mouse click somewhere else in the text field, the IME doesn't close. Then continuing to type produces text in the previous location. If you click on another element with the IME open and continue typing, then it will add the previous text to this new element.
I am thinking of other reasons we might want to open/close the IME, other than focus change. One example could be pressing Escape key. I wonder if OnFocus and OnBlur are the best names, it seems to tie our hands a bit for the future. What did you think about OnActivate/Deactivate?
I did not introduce these methods for the context; I am unsure how I feel about adding methods with a TextInputContext parameter straight to the context,
Yeah, it doesn't look like we need it in my view.
I can take a look at writing documentation for all of this, as there is a lot to cover (new "interface" to override, ways to utilize the text input handler for other stuff, text input context, default IME support, ...).
That would be amazing!
This reverts commit 853a777.
Thank you for the intensive review. I believe that I have addressed all the points by now. :) The issue with the IME is an excellent catch, and it caused me some trouble figuring it out. But after experimenting with Firefox and Chromium, I have noticed that their approach is straightforward; if you try to interact anywhere with your mouse, it will immediately cancel the composition by confirming the current string. It should be as simple as this: 869f0d0 Nevertheless, it is interesting that Windows does not emit any message when clicking outside the composition and candidate windows. Yet, if one tries to press the Escape key, it immediately cancels the composition. So, that case is automatically handled. I have followed your suggestion and replaced The classes should be better documented now. I will start working on the official documentation in the following days. |
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.
Thank you for the continued efforts, functionality wise it seems to work very well for me now! Just canceling the composition on mouse clicks seems like a good solution. Nice work. A couple comments remain.
By the way, have you seen this one: https://w3c.github.io/edit-context/
I came by this recently. I think it's quite interesting, looks like we've converged on a very similar design I would say. Maybe something to consider to align ourselves with for the future, or just take inspiration from, but I don't think we need to make any changes based on this now.
Thank you for the quick review and the detailed insight on why to avoid the shared pointer. The pull request should be complete. The linked draft looks interesting, although I am not exactly sure how this would link to RmlUi. If the EditContext API makes it to a final version and is considered to be implemented into RmlUi, there is much more to do, such as linking it to platforms. Nevertheless, I find it interesting how their |
The element's context is not available during destruction, so the call to `OnDestroy` was never executed. Instead, store the handler it was constructed with so that we can call `OnDestroy` with that.
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.
Awesome, I am happy with these changes, nicely done! It became a bit of a journey, hope all my comments weren't too overbearing. I think we ended up at a good place. :)
I added one commit at the end here, hope it looks okay to you.
About the EditContext API, I just found it interesting to compare to our own input context, could be interesting for something like #624. I think our API could just as well work with a collection of elements too, like for a rich text editor. Just a side note though.
I apologize for the delays. But I am happy to see the approval on this. I am currently working on documenting all of this. 🚀 One more comment on the EditContext API: I still find it confusing how it may link to the library, mainly since the provided examples use and talk only about canvases. If, however, it is supposed to be a proxy to the element itself, we would have to provide a getter to obtain the |
Thanks again for the pull request and your continued efforts, nice to see it merged! If I understand the EditContext API proposal, one of the use cases is to support things like IME for users that implement their own formatting utilities, such as rich text editors directly drawing into canvas elements. Then users can implement the "edit context" for their own formatter to take advantage of IME that the browser provides. Of course, we are in a bit of a reverse situation here, we want users to implement the IME backend, while the library provides the edit context. But I could also see someone wanting to implement a custom editor (e..g the rich editor mentioned above), then they could take advantage of the existing IME and just implement the edit context for their editor. |
One of the fundamental aspects of forms is IME. Yet, it is very often ignored and not adequately integrated into games. However, this is no surprise since Windows seems to be the only one with a proper system, which is not well documented!
After implementing such a system into our product and playing around with it for a little while, I decided not only to add a public interface for visual feedback (the initial intent of this pull request) but also, after a bit of discussion below, implement a system with a custom handling of the IME composition string. Right now, this is supported only on Windows.
The public interfaces for the text input method editor and the addition of
GetTextInputMethodEditor()
toSystemInterface
should also allow a custom implementation of such a system.Btw. the selection and IME API duplication is cumbersome and should be addressed in the future.