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

Text Input Method Editor #541

Merged
merged 42 commits into from
Jul 2, 2024
Merged

Conversation

ShawnCZek
Copy link
Contributor

@ShawnCZek ShawnCZek commented Nov 17, 2023

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() to SystemInterface 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.

@ShawnCZek
Copy link
Contributor Author

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.

@ShawnCZek
Copy link
Contributor Author

This is what the visual feedback looks like:

Solid line under CJK IME input

Windows usually uses a dashed or squiggly underline:

Squiggly line under CJK IME input

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.

@mikke89 mikke89 added enhancement New feature or request internationalization labels Nov 20, 2023
@mikke89
Copy link
Owner

mikke89 commented Nov 21, 2023

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.

  1. We currently have the Windows backend with IME support, did you try this out? Do you feel this is inadequate in some way?
  2. I think it's important that we don't walk down a path where we start implementing specific IME logic in terms of language rules and so on. Instead, we need to ensure that our API is written in such a way that it is flexible enough that it can be considered "done". Can you perhaps comment on that, can you see any ways this API needs continuous refinement?
  3. In line with the previous point, I believe any such API should mainly serve to tie this library to more IME-specific code in another library or API. What do you think?

@ShawnCZek
Copy link
Contributor Author

  1. Yes, I have tried the native IME support. Its behavior and design, however, do not meet my expectations. Below, you can see two recordings comparing the default demo and my current (complex) prototype. Please ignore the glitchy candidate window.
demo_61xjpGUUv1.mp4
eurotrucks2_WMCMJvH4zl.mp4
  1. This specific range may be used for any type of language or feature (e.g., the emoji picker on Windows). But in my demo, you may notice that I use the selection range for Hangul IME. Windows, on the other hand, uses a solid line.
  2. If well implemented, this works exceptionally well with Unreal Engine. Unfortunately, I cannot provide concrete implementation as I do not work with it. It may, however, be used for ITextInputMethodContext::UpdateCompositionRange().

Back to the second point. If we go by Unreal Engine, such input method context must do the following:

  1. Get the current selection range/caret position.
  2. Set the caret position.
  3. Retrieve the current text value.
  4. Set a text value in a range.
  5. Obtain the cursor position on the screen.
  6. Get the input element size.
  7. Update the composition range (e.g., for visual feedback).

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:

  1. ElementFormControlInput::GetSelection(...)/ElementFormControlTextArea::GetSelection(...)
  2. ElementFormControlInput::SetSelectionRange(a, a)/ElementFormControlTextArea::SetSelectionRange(a, a)
  3. ElementFormControl::GetValue(). The interface implementation should take care of retrieving only a certain range.
  4. A custom implementation using ElementFormControl::SetValue(...):
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 );
  1. Pass the parameter from SystemInterface::ActivateKeyboard(...) to the context.
  2. Element::GetBox().GetSize( Box::BORDER ).
  3. ...? This is not currently possible. Hence, this pull request.

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.

@mikke89
Copy link
Owner

mikke89 commented Nov 26, 2023

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.

@ShawnCZek
Copy link
Contributor Author

ShawnCZek commented Nov 27, 2023

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 IMEManager handling the front end, I am looking at hundreds of lines of code synchronizing the composition string and the previous text value, composition range and caret, displaying all possible options, possibly with a number, and deciding on when the composition string needs to be selected. This is impossible with such little exposure to the inner workings of the system.

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.

@mikke89
Copy link
Owner

mikke89 commented Dec 2, 2023

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.

@ShawnCZek
Copy link
Contributor Author

ShawnCZek commented Feb 14, 2024

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 TextInputMethodContext interface is based on my earlier comment.

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 SetIMEHighlight?


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.

@mikke89
Copy link
Owner

mikke89 commented Feb 26, 2024

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 line_height in the SystemInterface_Win32::ActivateKeyboard function, but only with the ime sample where we render the composition ourselves.

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 TextInputMethodContext could indeed become part of either the text elements themselves, or perhaps ideally, a single interface for the context which handles the correct focus. Moreover, we could also consider moving the ActivateKeyboard functions from the system interface and into this one. As a first step, implementing this in a separate sample is fine.

I quite like the term "composition range" you used in the sample, maybe we could name the function SetCompositionRange()? I would also be okay with SetIMEHighlight or possibly SetIMERange.

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.

@ShawnCZek
Copy link
Contributor Author

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 line_height, I am unsure whether I fully understand how this can be used. One of the CANDIDATEFORM's structure members is indeed rcArea, but that also requires the width of the input box, which we do not have, unless we use an artificial value.

I agree with implementing a separate interface, TextInputMethodContext, for text elements. The widget itself could, in theory, handle this. I do not fully understand what role ActivateKeyboard plays here as the user should implement this, and it should remain a part of the public interface. Moreover, how do we want to connect the context to the system interface? Should I create an interface for an "editor" like I did in the simple, which will be "globally" accessible from the system context?

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.

@ShawnCZek ShawnCZek force-pushed the ime_visual_feedback branch 2 times, most recently from e9a4d22 to b08f293 Compare March 16, 2024 16:42
@ShawnCZek
Copy link
Contributor Author

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 wchar_t offset.

// The cursor position is the wchar_t offset in the composition string. Because we
// work with UTF-8 and not UTF-16, we will have to convert the character offset.
int cursor_pos = IMEGetCursorPosition(imm_context);
std::wstring composition = IMEGetCompositionString(imm_context, false);
Rml::String converted = RmlWin32::ConvertToUTF8(composition.substr(0, cursor_pos));
cursor_pos = (int)Rml::StringUtilities::LengthUTF8(converted);
editor->SetCursorPosition(cursor_pos, true);

This is some dark magic, but that goes for the entire Imm API and WTF-16. :)

A new important addition is TextInputMethodEditor, which has become a part of the system interface. However, library users do not have to make any changes if they use the default Windows platform implementation. And if they have a custom platform, they have to make their system message handler, anyway, meaning they will probably want to create a custom IME system (like in my case). This is also why SetIMERange() remains publicly available from text input elements.

Anyway, I am still confused about what you wanted to do with ActivateKeyboard. Could you please elaborate on this compared to what I have done with implementing the IME editor and contexts?

@ShawnCZek ShawnCZek force-pushed the ime_visual_feedback branch from 83984ea to e022074 Compare March 16, 2024 23:00
@mikke89
Copy link
Owner

mikke89 commented Mar 18, 2024

Very cool, thanks a lot! I'm wrapping up some other things right now, I'll take a closer look at this soon :)

@mikke89
Copy link
Owner

mikke89 commented Apr 15, 2024

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:

Unifont:
Screenshot 2024-04-15 - unifont

System fonts:
Screenshot 2024-04-15 - system fonts

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:

  • I wonder if we can remove the separate IME sample and integrate it with one/some of the other samples.
  • Would be really interesting to see IME combined with the harfbuzz font engine / sample.
  • I seem to be encountering a bug, where if I add enough text so that the <textarea> overflows, then no text is shown.
  • I was a bit confused that it didn't work for me at first, but then I figured I didn't use a Win32 platform backend. We should probably make some note of the backend in the IME sample (or where we demo this if we don't keep it).

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.

@ShawnCZek ShawnCZek force-pushed the ime_visual_feedback branch from e022074 to 8e4b79b Compare April 24, 2024 21:27
@ShawnCZek
Copy link
Contributor Author

ShawnCZek commented Apr 24, 2024

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:

  • The sample is indeed pretty simple. However, IME currently works only on Windows and requires fallback fonts supporting different character sets to function correctly, which we have to load via code; loading system fonts is impossible via RCSS.
  • I do not know what the benefit of Harfbuzz is, especially with IME, because I have never used it. Do you think it is helpful to make such a sample or try this any further?
  • This seems to be a styling issue. I fixed it in 42df3fa.
  • This could be resolved on the CMake level by not including the sample when the Win32 backend is not used. Ideally, it would be nice to support more backends, although that is more complicated for me personally as I have never used those. I can leave it to others who have been (un)fortunate enough to work with them. :)

Rebasing was not very complicated. The only issue is the macOS build is failing for some reason. I would need to look into this.

@ShawnCZek
Copy link
Contributor Author

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.

@ShawnCZek ShawnCZek changed the title Add visual feedback for IME composition in text inputs Text Input Method Editor Apr 25, 2024
@ShawnCZek ShawnCZek marked this pull request as ready for review April 25, 2024 16:44
@mikke89
Copy link
Owner

mikke89 commented Apr 25, 2024

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 cmake branch works well with these changes, so we'll just wait for that: https://github.com/mikke89/RmlUi/actions/runs/8581424487/job/24273174801

@ShawnCZek ShawnCZek force-pushed the ime_visual_feedback branch from cf21e06 to a7c6685 Compare April 26, 2024 12:10
Copy link
Owner

@mikke89 mikke89 left a 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.

Samples/basic/ime/src/SystemFontWin32.cpp Outdated Show resolved Hide resolved
Samples/basic/ime/data/ime.rml Outdated Show resolved Hide resolved
Backends/RmlUi_Platform_Win32.cpp Outdated Show resolved Hide resolved
Source/Core/Elements/WidgetTextInput.h Outdated Show resolved Hide resolved
Source/Core/Elements/WidgetTextInput.h Outdated Show resolved Hide resolved
Include/RmlUi/Core/TextInputMethodEditor.h Outdated Show resolved Hide resolved
Include/RmlUi/Core/TextInputMethodEditor.h Outdated Show resolved Hide resolved
Include/RmlUi/Core/TextInputMethodEditor.h Outdated Show resolved Hide resolved
Source/Core/Elements/WidgetTextInput.cpp Outdated Show resolved Hide resolved
Include/RmlUi/Core/TextInputMethodContext.h Outdated Show resolved Hide resolved
@mikke89
Copy link
Owner

mikke89 commented May 23, 2024

Oh yes, looks like we have come fill circle indeed!

Sounds good, I like the suggested TextInputHandler, so feel free to go ahead with that.

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.
@ShawnCZek
Copy link
Contributor Author

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:

  1. Introduced TextInputHandler which is context-dependent.
    • It can be customized during the context initialization.
    • A default implementation is provided by passing an empty TextInputHandler object.
    • Rml::SetTextInputHandler() and Rml::GetTextInputHandler() have been introduced.
    • 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, as it sounds like the context may do much more behind the scenes. However, I can add those, probably called FocusTextInput() and BlurTextInput().
  2. TextInputMethodEditor has been moved to the backend based on TextInputHandler. It is passed to all contexts via the above-mentioned Rml::SetTextInputHandler() function, called from the backend constructor. I did not find a better place for this.
  3. The IME implementation is forced on the user out of the box, allowing the removal of all the defensive if statements.

Except for the possible addition of the two methods for Context, this pull request should be completely ready. 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, ...).

Copy link
Owner

@mikke89 mikke89 left a 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!

Backends/RmlUi_Platform_Win32.cpp Outdated Show resolved Hide resolved
Include/RmlUi/Core/TextInputHandler.h Outdated Show resolved Hide resolved
Include/RmlUi/Core/TextInputHandler.h Outdated Show resolved Hide resolved
Include/RmlUi/Core/TextInputContext.h Show resolved Hide resolved
Include/RmlUi/Core/TextInputHandler.h Outdated Show resolved Hide resolved
Backends/RmlUi_Platform_Win32.cpp Outdated Show resolved Hide resolved
Backends/RmlUi_Platform_Win32.cpp Show resolved Hide resolved
Include/RmlUi/Core/TextInputHandler.h Outdated Show resolved Hide resolved
@ShawnCZek
Copy link
Contributor Author

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 OnFocus/OnBlur with OnActivate/OnDeactive. While I liked the previous names better, as they are much clearer, I understand your concerns. And renaming anything in the future is difficult.

The classes should be better documented now. I will start working on the official documentation in the following days.

Copy link
Owner

@mikke89 mikke89 left a 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.

Source/Core/Elements/WidgetTextInput.cpp Outdated Show resolved Hide resolved
Include/RmlUi/Core/TextInputHandler.h Outdated Show resolved Hide resolved
@ShawnCZek
Copy link
Contributor Author

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 EditContext can be attached to multiple elements.

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.
Copy link
Owner

@mikke89 mikke89 left a 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.

@ShawnCZek
Copy link
Contributor Author

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 TextInputContext implementation.

@mikke89 mikke89 merged commit d2ce132 into mikke89:master Jul 2, 2024
32 checks passed
@mikke89
Copy link
Owner

mikke89 commented Jul 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants