search

libhangul / ibus-hangul

The hangul engine for IBus
GNU General Public License v2.0
66 stars 18 forks source link

Do not commit text when reset signal is received #85

Closed epico closed 5 years ago

epico commented 5 years ago

ibus-hangul uses ibus_engine_update_preedit_text_with_mode() to commit the preedit text by ibus-daemon and now focus-out and mouse click are handled by ibus-daemon and ibus clients.

BUG=https://github.com/ibus/ibus/issues/1980

choehwanjin commented 5 years ago

This code is too complicated. I thought that there is some simple api for that. Could you bump up the version required in configure.ac to 1.5.20, and just call hangul_ic_reset in ibus_hangul_engine_reset()?

Please add issue URL on git comment.

fujiwarat commented 5 years ago

@choehwanjin I think the patch follows your suggestion to get the version in runtime. Normally it would be better to check IBUS_CHECK_VERSION() than checking in configure in build time. I also think it's better to support the previous ibus version too in the latest ibus-hangul likes #68 .

changwoo commented 5 years ago

@choehwanjin I think the patch follows your suggestion to get the version in runtime. Normally it would be better to check IBUS_CHECK_VERSION() than checking in configure in build time. I also think it's better to support the previous ibus version too in the latest ibus-hangul likes #68 .

IIUC, with IBUS_CHECK_VERSION() on compile time, ibus-hangul built on libibus 1.5.19 will become broken after ibus gets upgraded to 1.5.20.

choehwanjin commented 5 years ago

@fujiwarat I thought there is a simple API to check version, but there isn't. And the code is more complicated than I thought. So I changed my mind.

fujiwarat commented 5 years ago

Both IBUS_CHECK_VERSION() and configure are executed on compile time and I think IBUS_CHECK_VERSION() with ibus-hangul 1.5.2 will work with ibus 1.5.19 and 1.5.20 but ibus-hangul have to be rebuilt if ibus 1.5.19 is upgraded to ibus 1.5.20. I think configure.ac with ibus-hangul 1.5.2 will work with ibus 1.5.20 but users have to upgrade ibus 1.5.y to 1.5.20 or later.

Probably it depends on what's are the new features between ibus-hangul 1.5.1 and 1.5.2? OK, it's up to the ibus-hangul maintainers.

epico commented 5 years ago

Okay, we respect the upstream decision.

choehwanjin commented 5 years ago

@fujiwarat OK, I understand. I will follow your suggestion. @epico Please update your patch.

epico commented 5 years ago

After discussion with Takao Fujiwara, I switch to use IBUS_CHECK_VERSION in C code now.

Please review this patch again, thanks!

fujiwarat commented 5 years ago

Thank you.

epico commented 5 years ago

Please review it again, thanks!

changwoo commented 5 years ago

It builds and works in 1.5.19. have not check how it works in 1.5.20.

changwoo commented 5 years ago

It builds and works in 1.5.19. have not check how it works in 1.5.20.

I tested this patch w/ ibus 1.5.21. The one problem is ibus-daemon does not update the "version" GSettings in GNOME session. I see --panel disable option in ibus-daemon and maybe this is why.

After I set the "version" as "1.5.21" manually, GTK preedit reset and commit worked as expected.

changwoo commented 5 years ago

The one problem is ibus-daemon does not update the "version" GSettings in GNOME session. I see --panel disable option in ibus-daemon and maybe this is why.

Logging-out and logging-in a new GNOME session (instead of ibus restart) updates the settings. I'm not sure why but I'm OK with this.

This PR looks good. Thanks to all who did this work.

epico commented 5 years ago

Thanks for the review!

fujiwarat commented 5 years ago

I tested this patch w/ ibus 1.5.21.

If you upgrade ibus, you need to restart ibus-daemon and ibus-hangul is also restarted. I think GSettings does not require to restart the desktop session. You might have to run glib-compile-schemas after you update ibus gschema file.

epico commented 5 years ago

@fujiwarat

I also meet the version problem in gnome session, the version value is empty.

Could you fix this issue?

fujiwarat commented 5 years ago

@changwoo @epico Right. I forgot the original usage of the GSettings "version". It has been used to check the previous IBus version if any migration is needed.

While I can move the current "version" to "prev-version" and set the static "version", Probably I think it's better to add a new API const gchar *ibus_get_version() than the GSettings values?

Sorry for confusing the usage.

changwoo commented 5 years ago

Probably I think it's better to add a new API const gchar *ibus_get_version() than the GSettings values?

If the new version info API is added in 1.5.22 or later and used by ibus-hangul, ibus-hangul doesn't have to check >= 1.5.20 in run-time. :) It's good to add such API for the future other uses though.

epico commented 5 years ago

@fujiwarat Maybe static "version" value is okay.

fujiwarat commented 5 years ago

However if the string is static, using GSettings might be strange since the value is read-write? If you'd check the version in ibus 1.5.20, how about running ibus version command?

epico commented 5 years ago

I guess it may make code more complicated, and it depends on the output format of ibus version. It needs to use g_spawn_async_with_pipes function.

BTW, which version of ibus starts to support ibus version command?

fujiwarat commented 5 years ago

BTW, which version of ibus starts to support ibus version command?

It has been since 1.5.1.

Probably you could check the filename in /usr/lib/libibus-1.0.so.5..* .

epico commented 5 years ago

The filename may not reliable.

@fujiwarat, Could you set the version in ibus daemon? or set the version in gnome-shell is better?

fujiwarat commented 5 years ago

Do you mean --version?

# ibus-daemon --version
ibus-daemon - Version 1.5.20
epico commented 5 years ago

Okay, I create another pull request to check the output of ibus version command. URL: https://github.com/libhangul/ibus-hangul/pull/87

Please review it again, thanks!

epico commented 5 years ago

Pull request #87 is merged, close this pull request.