-
Notifications
You must be signed in to change notification settings - Fork 654
Introduce minimal API for text keyboard request with text area and cursor support #857
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
base: master
Are you sure you want to change the base?
Introduce minimal API for text keyboard request with text area and cursor support #857
Conversation
4c66e79 to
9fa27a6
Compare
b34ef39 to
688591b
Compare
e082015 to
9efc7fe
Compare
|
This discussion started in #852, but it actually relates much more to this PR. PART 1CLICK TO SHOWPavelSharpNews. In the current version of this PR, keyboard input for all What do you think about this approach? sleeptightAnsiCI'll take a look into it. Callback would be perfect, but I don't want to break ABI/API accidentally. I suspect I simply missed some case for When it comes to font scaling, this code is not mine, it comes from other demos and I didn't dare to touch it. I'll see what can be done here. I also just realized that I did broke commit history. There should be two commits, not one... I probably smashed those during latest force-push. PART 2CLICK TO SHOWPavelSharp@sleeptightAnsiC, maybe I’m missing something, but after looking deeper into this issue, I’m convinced that the only truly safe and future-proof solution for handling text input and virtual keyboard behavior is to add a callback. Ideally behind a macro guard so it doesn’t break the ABI or API. Here are my main arguments:
I’d really like to hear what you think about this, @rswinkle and @RobLoach. If nobody minds, I’ll go ahead and start working on it soon❤️ sleeptightAnsiC
I don't see how adding a callback would solve any issue here. It could be a nice addition, but the callback needs to be call'ed by something. I'm not gonna introduce any callback as part of this PR and I won't change my mind about it. PavelSharp
I'm sure it's obvious. So, my plan is to introduce a callback that would be invoked inside It’s also important to note that
I believe I’ve given enough reasons why a new callback is preferable. Modifying sleeptightAnsiC
If that's so simple, then just move the edit state de/activation to this one function, and the problem will be "fixed". The state changes are already here, the callback would be a new thing. One does not contradict the other, but the old implementation should be fixed. DISCLAIMER: From now on I'm gonna ignore any text that has a sign of AI/ChatGPT use (sorry)PART 3CLICK TO SHOWrswinkle
I have to agree with @sleeptightAnsiC on this.
I would not call either my approach or the current approach fragile. As sleeptightAnsiC said, all approaches at their optimal/minimal state amount to the same thing, and in fact the callback method probably requires the most in actual lines changed. The logic is not/would not be in the callback. It is a matter of Nuklear internal state regardless. Knowing where/when to insert the callback is the same knowledge you would need for any other method.
Again, these are Nuklear bugs that need to be fixed in any case and the code changes are very small. It's a matter of discovery/precision.
The request and release does not happen at the widget level, that's part of the issue. It has to be aggregated over all the windows/widgets over the whole frame being rendered, and at that point, you know if either Start or Stop should be called based on whether it changed from the previous. Whether you use my method, adding a couple of members to try to simplify the test by collating as we go, or try to get the same result using a function like nk_is_any_active() combining hopefully all necessary checks (or both), the solution is the same. Ideally all widgets using text input should use the same method directly or (since that's obviously not the case) they should pass that state along to the text_edit state as the "master" state.
There is no breaking change regardless. Nuklear is not used as a dynamic library that will be updated separately from the application(s) that use it. It is meant to be compiled statically. And the changes are not what I would consider "breaking": Either a couple added field to an internal structure, an extra utility function meant almost exclusively for backend implementers not average users, or both. No change to the existing API, and nothing removed that could break someone's code.
I still don't really understand the point of this function but all the logic above applies here too afaict. The difficulty is in getting the information in the first place.
None of them are hacky workarounds. As I said all would require roughly the same logic/changes, the callback would just require a bit more code for the macro stuff at the top. My first attempt at doing this was a macro based callback system a la NK_IS_WORD_BOUNDARY() which I worked on a while back. It was two optional macros NK_ON_EDIT_FOCUS()/NK_ON_EDIT_UNFOCUS(). I ended up deleting it and replacing it twice, ending up on my current version because I realized everything that I discussed above. Not only do we not want or need to call Start/StopText() within Nuklear (even indirectly), but everything boils down to correctly and efficiently tracking the boolean state so the backend/user can do it themselves if they want to. What should be documented is all the knowledge about how that state is tracked which is what someone implementing a new text input widget not based on the nk_do_edit() family needs to know. As long as they remember the few lines to set the state appropriately then everything would just work.
See above. It's not that many changes and you would have to touch the same pieces of the code for the callback method. PART 4CLICK TO SHOWPavelSharp
I believe that the sleeptightAnsiC
I mistaken However, I don't believe we can take user-defined text widgets into consideration, if those aren't being implemented via Your solution from #857 also triggers the PavelSharp
Let's imagine a user-defined widget that wants to notify Nuklear's input system (and backend), that it is about to receive text keyboard input. In this case, the widget is responsible for calling A clear example of this is The main point is that [NEW PART]
In fact, the existence of such widgets is also assumed by @rswinkle in his message:
sleeptightAnsiCI've taken a better look at #857 and I'm a bit confused.
Why would people follow this very specific "concept", if they're not even following an already established solution like
This is not a notification, no one is getting notified about anything. The user of #857 would still have to pull that information manually from This really feels like repeating the old mistake. We already proven that widgets are not to be trusted with setting the context state correctly, and against all reasoning, this new "concept" introduces yet another state that we have to track and care about. It does not create any convention that custom widgets could safely follow. I don't get it. All your previous messages were about a "callback" system (I see 43 matches when I search for this word on this page). You've tried very hard to convince us to "calback" solution, so why this new thing ended up being another manual "state" update? I'm really confused here. Note that I only disagreed about callbacks being a part of this PR, I still thought you would go for it later in another PR (#857). PavelSharp
That’s not quite right. The backend is the one that needs to read this information, the user doesn’t have to worry about it.
Yes, if a user wants to create a custom, non-standard text-input widget, they should use this new simple API. In this very rare situation, this behavior is completely normal, and it has already been mentioned here(quoting it for the second time):
Next
It’s possible that the earlier arguments weren’t convincing, because #857 works very well and very stable. I haven’t been able to find any issues, even when testing different combinations of clicks and focus changes.
First, this new state exists only until the next call to Second, please read my quote again:
So, the rule is very simple.
In the end, my opinion changed. You may want to look more closely at #857, where it is explicitly mentioned:
as well as:
|
Please, notice that I'm writing everything here in good-faith. This topic has a long and complicated story. No harm intended.EDIT: hah, Pavel posted his message just few seconds before mine. See the Due to many concerns, I did not want to introduce any complicated TextInput handling during #852 and decided to go with very conservative and minimal solution that would barely change the library code, just to make it possible to hook Pavel tried to convince us that the "callback" system is the only true solution to this problem, and probably it is, but back then I disagreed, because of the reasons mentioned in the previous paragraph, and because there is already a lot of existing functionality that we could try to reuse. However, I did agree that this can be solved later during different PR, and this implies that "callback" solution could work (just not as a part of #852). Now, my current stance on this PR is as follows: It is NOT a "callback" system, but instead an another "state tracking" that introduces yet another widget convention. I believe that the solution should EITHER be a "callback" system OR some fixes to the OLD state tacking. I think this comment reflects my point of view well #852 (comment) (it's a reply to longer thread):
This was later answered with some explanations, here #852 (comment) , but I don't think I agree with everything. There is a lot of information, so I'm sorry if I missed something. Despite my current disagreement, I really really want this feature in, and I even removed any possible conflicts from #852 that could make it harder to introduce, just to ensure that Pavel (or someone else) will have a free hand when implementing this. I simply think the current approach in this PR is not entirely correct, and it may cause people to re-implement this whole convention YET AGAIN in the future, just like what we're trying to do right now. I just wanted to make this whole situation and my reasoning clear. Again, I'm not trying to sabotage this change, I just want everyone to understand the complexity of this problem. You can see how much time was put into this during #852 and yet we decided to not fix it back then. |
|
Implementation note: We probably also want to support something like SDL_StartTextInputWithProperties, where the caller needs to know a bit more about the kind of current text input. Nuklear already has something like this in a form of nk_filter_*, but we also need to consider user-provided filters. |
🎉🥳 I’m happy to propose a very small but useful API addition that allows Nuklear to provide complete information about the active text input area and cursor position. This data can be consumed by backends to enable functions such as
SDL_SetTextInputAreaMini FAQ
Q: Why SDL_SetTextInputArea is important?
Answer
SDL_SetTextInputAreainforms the operating system where text is currently being entered.This is crucial for IME (Input Method Editor) systems.
For example, on Windows with the Japanese IME enabled, the OS shows a small candidate list window near the cursor when typing. Without accurate text area information, this window appears in the wrong place. With this API, the IME candidate window is correctly positioned on top of the actual editing location.
This greatly improves the usability of Nuklear for logographic languages such as Japanese, Chinese, and Korean. And that's just one example we haven't even touched mobile devices yet!
Q: Why this architecture?
Answer
The chosen design is inspired by Dear ImGui.
ImGui demonstrates that callback-based control can be unnecessarily heavy, and that a much simpler pattern works better in immediate-mode GUI systems: the backend polls the desired text input state every frame.
The key idea:
text_want) and useful data(text_areaandtext_cursor).No persistent widget ownership logic is required, and the changes to Nuklear’s core remain extremely small.
Q: Why we believe this approach is robust
Answer
This design follows what I call a proof system:
nk_input_begin, text input is considered disabled.text_wantflag.This approach guarantees that no stale state carries over between frames, and avoids the need for any complex widget-activation logic. The text area and cursor rectangles remain simple auxiliary metadata.
This also includes a patch (written quickly, so not yet C89 compatible) for the existing SDL3 PR. I highly recommend trying this patch, as it visualizes debug rectangles for the active text field and cursor. Japanese keyboard layout is recommended for testing on Windows.
CLICK ME