| Summary: | SDL_SendEditingText and SDL_SendKeyboardText cut off IME input more than 32 bytes. | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Hak Matsuda <hakuro> |
| Component: | events | Assignee: | Sam Lantinga <slouken> |
| Status: | ASSIGNED --- | QA Contact: | Sam Lantinga <slouken> |
| Severity: | normal | ||
| Priority: | P2 | CC: | alex, hqm03ster, icculus, ttakah+sdl, xenotron007 |
| Version: | 2.0.3 | ||
| Hardware: | x86 | ||
| OS: | Other | ||
| Attachments: |
patch to implement SendEditEX for 3006, 3421, 4782, and 4783 for windows and fcitx (not yet for ibus)
patch to implement SendEditEX for bugs 3006, 3421, 4782, and 4783 on windows and fcitx (not yet for ibus) |
||
|
Description
Hak Matsuda
2015-06-10 03:03:46 UTC
How big are the strings likely to be? One solution to this is to have the application free those strings. Another solution would be to keep some number of them cached and valid, but that could cause an application to crash or see the wrong text if the events are not handled for a long time. In case of Japanese, it's likely in a range of 64 bytes ~ 256 bytes in most of use cases (I would imagine it's similar in other languages that has a context based character transform mechanism). I would prefer former option rather than to have a chance to lose them. (In reply to Sam Lantinga from comment #1) > How big are the strings likely to be? > One solution to this is to have the application free those strings. Another > solution would be to keep some number of them cached and valid, but that > could cause an application to crash or see the wrong text if the events are > not handled for a long time. I haven't looked, but could we just split them up into multiple TEXTINPUT events? That would keep the existing API/ABI intact. --ryan. SendKeyboardText could, but the editing text is an in-place temporary string and each event replaces the previous text. If it's easier to fix, adding new definition of SDL_TEXTEDITING_EXTRA to send extra bytes (multiple times depending on the length) in addition to existing message and let the receiver to concatenate them, would work? In the iBus code, I worked around this by sending multiple input / editing events in a row if the text is more than 32 bytes. You'd have to concatenate all the editing events that were received in a single PumpEvents / PollEvent loop to get the full replacement editing string. Not sure that is the best approach though. *** Bug 2857 has been marked as a duplicate of this bug. *** I think one could just add an SDL_TEXTEDITING_EX event which appends text to those sent by SDL_TEXTEDITING and updates the cursor accordingly. That way, existing applications could just ignore that and work with the old clamped text. We could also do the same thing that we do for drag-and-drop events, where we create a new event that's disabled by default, and the text is dynamically allocated and needs to be freed by the code processing the event. Is there a consensus regarding the solution? This latter proposal is probably the best option. If we have to add new events anyway (due to binary compatibility) and have to adjust the app code to handle them then we shouldn't make our lives more difficult by creating new events that have to cooperate with the old ones. How to allow the opt-in? A function call (like SDL_EnableTextInput2()) that turns all SDL_TEXTEDITING and SDL_TEXTINPUT events into their v2 form (SDL_TEXTEDITING_2 and SDL_TEXTINPUT_2?) or an alternative like an SDL_StartTextInput2() function so different parts of the codebase can choose from v1 or v2 events by calling either SDL_StartTextInput() or SDL_StartTextInput2(). That way older parts of the app could still use the old SDL_StartTextInput() and work with the older events. Having both versions of the events in the same application might add more trouble than benefit due to the centralised nature of event handling in a lot of SDL apps so having a global switch is probably a safer choice. *** Bug 4774 has been marked as a duplicate of this bug. *** (In reply to tamo from comment #11) > *** Bug 4774 has been marked as a duplicate of this bug. *** But 4774 has a buildable patch to malloc the text (and thus needs freeing). If you should add another event to push extra information on editing event, please consider separating readingstring from composition (editing) text. Currently SDL puts readingstring into composition text at cursor's position. http://hg.libsdl.org/SDL/file/08db6a6f6c23/src/video/windows/SDL_windowskeyboard.c#l771 But they are different things. I don't think they should be mixed. For example, target range, which I suggested you add to the event in bug 4783, refers to raw composition text, not the mixed text. If readingstring were a separate field, the patch for bug 4783 would have been a little simpler. Composition is composition, reading is reading. I hope they will be provided in their whole length, as individual variables. Created attachment 3958 [details] patch to implement SendEditEX for 3006, 3421, 4782, and 4783 for windows and fcitx (not yet for ibus) (In reply to Alex Baines from comment #6) > In the iBus code, I worked around this by sending multiple input / editing > events in a row if the text is more than 32 bytes. As mentioned in comment #4, the current SendEditingText implementations for ibus and fcitx are broken. They try to iterate a long editing text, but it doesn't work. I wrote an editx for fcitx. Although this patch includes some lines for ibus, they must be broken, bogus, because I haven't successfully set up ibus on my WSL ubuntu and I haven't tested them at all. Created attachment 3962 [details] patch to implement SendEditEX for bugs 3006, 3421, 4782, and 4783 on windows and fcitx (not yet for ibus) (In reply to tamo from comment #14) > I wrote an editx for fcitx. I made it a little more readable. I tested it with testime. (This patch modifies it so that it underlines the target text. Also, space key is disabled because fcitx uses Ctrl+space to trigger IME. Press backspace to exit.) The following statement is still true. > Although this patch includes some lines for ibus, they must be broken, > bogus, because I haven't successfully set up ibus on my WSL ubuntu and I > haven't tested them at all. |