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

A few small issues with the Ibus IME code #1583

Closed
SDLBugzilla opened this issue Feb 10, 2021 · 0 comments
Closed

A few small issues with the Ibus IME code #1583

SDLBugzilla opened this issue Feb 10, 2021 · 0 comments

Comments

@SDLBugzilla
Copy link
Collaborator

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: HG 2.1
Reported for operating system, platform: Linux, x86_64

Comments on the original bug report:

On 2014-07-27 15:48:10 +0000, Alex Baines wrote:

Created attachment 1789
[Patch 1/2] Fix shadowing + Send event when candidate list closes.

Here are a couple of patches that fix a few issues with the current ibus IME code:

  • Fix variable name shadowing in SDL_ibus.c
  • Send a blank SDL_TextEditingEvent when the candidate list closes due to pressing backspace.
  • Take the window border size into account when positioning the candidate list.

On 2014-07-27 15:48:45 +0000, Alex Baines wrote:

Created attachment 1790
[Patch 2/2] Position the candidate list correctly.

On 2014-07-27 17:25:22 +0000, Sik wrote:

Just tested it, I can confirm this patch works.

On 2014-07-27 18:13:54 +0000, Alex Baines wrote:

Created attachment 1791
[Patch 2/2] Position the candidate list correctly.

Better version of patch 2/2 that will compile even with --disable-video-x11.

On 2014-07-30 00:02:09 +0000, Alex Baines wrote:

Created attachment 1795
Extra patch: Add a hint so that IBus works better with apps that don't handle TEXTEDITING events.

I just made a new patch that adds some more IBus functionality and I thought I should put it here so that I don't spam the bug tracker with new IBus related things.

It adds a SDL_IBUS_EDITING_EVENTS hint which lets SDL know that you want to render the editing text yourself.

With this patch, IBus does the editing stuff in its own UI by default. This works a lot better with applications that only look at the TEXTINPUT events and not the TEXTEDITING ones as well.

If you want to get the TEXTEDITING events and do the rendering of text as it is being editing then you can set this hint to 1 with SDL_SetHint(SDL_HINT_IBUS_EDITING_EVENTS, "1");

If this should be put in a separate bug then let me know.

On 2014-07-30 01:03:16 +0000, Sik wrote:

But doesn't calling SDL_StartTextInput basically imply that you will handle the SDL_TEXTEDITING events? Also this issue isn't specific to iBus as far as I know, all systems that support IME-style input suffer from it.

So I see two issues from this patch:

  1. It breaks compatibility with programs written before it, since they won't explicitly set the hint even though they already handle the situation.

  2. It's iBus-specific, when it's trying to solve a problem that happens on all platforms.

Probably there's a better solution.

On 2014-07-30 01:16:37 +0000, Alex Baines wrote:

(In reply to Sik from comment # 5)

But doesn't calling SDL_StartTextInput basically imply that you will handle
the SDL_TEXTEDITING events?

Well, you also have to call it to get the TEXTINPUT events, and for a lot of programs that's all they bother to handle.

Also this issue isn't specific to iBus as far as
I know, all systems that support IME-style input suffer from it.

Maybe, I'm not that familiar with how other IMEs work with SDL. I think the ibus internal editing is very useful though, for programs already out there that don't handle the editing events, ibus will "just work" with the new SDL lib swapped in.

So I see two issues from this patch:

  1. It breaks compatibility with programs written before it, since they won't
    explicitly set the hint even though they already handle the situation.

True, but since IBus support has only recently been added I don't see this being a huge deal. (It's only available in HG currently). You can also override it by setting the SDL_IBUS_EDITING_EVENTS environment variable to 1.

  1. It's iBus-specific, when it's trying to solve a problem that happens on
    all platforms.

Since iBus includes the functionality to do the editing itself I don't see why we shouldn't expose it in SDL somehow, even if other IMEs don't.

Probably there's a better solution.

Maybe, this is something that works without adding any new APIs though. Perhaps reversing the default, so that you have to set the hint to get iBus to do the work would be better.

On 2014-07-30 01:29:31 +0000, Sik wrote:

(completely forgot about the reply button)

(In reply to Alex Baines from comment # 6)

True, but since IBus support has only recently been added I don't see this
being a huge deal. (It's only available in HG currently). You can also
override it by setting the SDL_IBUS_EDITING_EVENTS environment variable to 1.

Programs that had already added IME support for the systems where it worked, they would automatically start using iBus if rebuilt even with the same source code (just updating SDL). This patch would require changing the program source code to have the same behavior as normal.

Maybe, this is something that works without adding any new APIs though.
Perhaps reversing the default, so that you have to set the hint to get iBus
to do the work would be better.

Probably, though changing the name to not be iBus specific would be nice too (in case this is implemented for other IMEs, or even emulated).

On 2014-07-30 02:01:39 +0000, Alex Baines wrote:

Created attachment 1796
Extra patch: Add a hint to allow IMs to work better with apps that don't handle TEXTEDITING events.

Programs that had already added IME support for the systems where it worked,
they would automatically start using iBus if rebuilt even with the same
source code (just updating SDL). This patch would require changing the
program source code to have the same behavior as normal.

Ok, that's a good point. What about changing the default then? That way it should work fine with those programs.

Also users could set the hint themselves as an environment variable for alredy-compiled programs that don't look at the editing events.

Probably, though changing the name to not be iBus specific would be nice too
(in case this is implemented for other IMEs, or even emulated).

I suppose changing the name is a good idea too, in case other IMEs want to do the same thing.

Here's a new version of the patch that swaps the default, and renames the hint.

On 2014-07-30 02:53:09 +0000, Sik wrote:

Yeah, this seems better regarding those issues. Nothing else I can think of at the moment, so this should be fine I believe.

On 2014-08-19 22:40:50 +0000, Alex Baines wrote:

Created attachment 1832
[Patch 1/2] Position the candidate list correctly.

Updating the patches to apply with the latest hg revision.

The shadowing has been fixed by someone else, so I've put the HidePreeditText signal handling that was originally in the shadowing-fix patch along with a few more minor changes in a new patch.

Also I swapped the ordering of the patches by mistake, so the candidate list position fix should be applied first now.

On 2014-08-19 22:42:17 +0000, Alex Baines wrote:

Created attachment 1833
[Patch 2/2] Handle HidePreeditText IBus signal, clean up other IBus stuff as listed in the patch header.

On 2014-08-19 22:44:39 +0000, Alex Baines wrote:

Created attachment 1834
Optional Extra patch: Add SDL_IM_INTERNAL_EDITING hint to let IBus handle showing the preedit text.

On 2014-08-20 05:01:37 +0000, Sam Lantinga wrote:

These patches look great, thanks!
https://hg.libsdl.org/SDL/rev/ed277c1c9e7f
https://hg.libsdl.org/SDL/rev/6454f71d6f15
https://hg.libsdl.org/SDL/rev/56d712662a82

I changed the name of the hint to match SDL IME naming convention:
https://hg.libsdl.org/SDL/rev/064ea0b1275c

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

No branches or pull requests

1 participant