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

ibus: 1.5.24 -> 1.5.26 #164105

Merged
merged 2 commits into from
Mar 15, 2022
Merged

ibus: 1.5.24 -> 1.5.26 #164105

merged 2 commits into from
Mar 15, 2022

Conversation

ncfavier
Copy link
Member

@ncfavier ncfavier commented Mar 14, 2022

https://github.com/ibus/ibus/releases/tag/1.5.25
https://github.com/ibus/ibus/releases/tag/1.5.26

Upstream added a dependency on systemd only to get the pkg-config variable systemduserunitdir, which is wrong for our purposes (we only want /lib/systemd/user without the systemd prefix) so we can drop the dependency.

Also enabled GTK 4 in hope to fix #163683 (cc @jian-lin, not sure how to test this)

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ncfavier
Copy link
Member Author

@ofborg test installed-tests.ibus

@bobby285271
Copy link
Member

Tested ibus-libpinyin on GNOME VM on de41ff40daa68a7f59d81e40e2cfb7920e97de34, Chinese input works on gedit (gtk3) and qownnotes (qt5) but does not works on gnome-text-editor (gtk4)

I did not do things other than adding

i18n.inputMethod = {
  enabled = "ibus";
  ibus.engines = with pkgs.ibus-engines; [ libpinyin ];
};

And adding intelligent pinyin to input sources (gnome-control-center, see the first screenshot)

Maybe some nixos modules also needs update, did not test yet.

@bobby285271
Copy link
Member

bobby285271 commented Mar 14, 2022

Looks like there is https://gitlab.gnome.org/GNOME/gtk/-/issues/1181 so there is no gtk4-query-immodules

@jtojnar
Copy link
Contributor

jtojnar commented Mar 14, 2022

Trying env GTK_PATH=$(nix-build -A ibus)/lib/gtk-4.0 $(nix-build -A gnome-text-editor)/bin/gnome-text-editor in this branch works for me (with Japanase (mozc) & ctrl-shift-e in Czech). And it does not work without the GTK_PATH. The only difference I noticed comparing it with gedit (GTK 3) is that the emoji character chooser is placed onto a wrong screen with GTK 4.

So it should be as simple as adding the ibus package to systemPackages, ensuring lib/gtk-4.0 is in pathsToLink, and adding that to

GTK_PATH = [ "/lib/gtk-2.0" "/lib/gtk-3.0" ];
.

@jtojnar jtojnar added this to In progress in GNOME 42 via automation Mar 14, 2022
@ncfavier
Copy link
Member Author

Done, not sure where the pathsToLink should go

@jtojnar
Copy link
Contributor

jtojnar commented Mar 14, 2022

I would put it to nixos/modules/programs/environment.nix as well.

Currently we are also getting it from

"/lib" # FIXME: remove and update debug-info.nix
but that will ideally change.

@ncfavier
Copy link
Member Author

OK, I also added /lib/gtk-{2,3}.0 since they're also in GTK_PATH.

- systemd >= 0.7.5
- ])
- AC_SUBST([SYSTEMD_USER_UNIT_DIR], [`$PKG_CONFIG --variable systemduserunitdir systemd`])
+ AC_SUBST([SYSTEMD_USER_UNIT_DIR], [/lib/systemd/user])
Copy link
Contributor

@jtojnar jtojnar Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use ${prefix}/lib/systemd/user here, instead all the hoops above? (Thinking ${} is the correct interpolation scheme in here, but not completely sure, maybe also try $(), and either of those with doubled $.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there is an alternative of keeping systemd and overriding the path using environment variable:

PKG_CONFIG_SYSTEMD_SYSTEMDUSERUNITDIR = "${placeholder "out"}/lib/systemd/user";

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh. Lost myself in the autohoops.

Which way do we prefer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way is fine, though if we keep it in a patch, it should probably go to a separate patch file. fix-paths.patch is usually refers to runtime paths in the code, rather than installation paths (yeah, the name is not good).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh, that sounds like an artificial difference? I think "patching paths due to Nix quirks (or non-Nix quirks)" is a reasonable category

Copy link
Contributor

@jtojnar jtojnar Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not say the difference is artificial, though it is hard to articulate. In this case, I would even consider it an upstream bug, see https://www.bassi.io/articles/2018/03/15/pkg-config-and-paths/. Maybe passing --define-variable 'prefix=${prefix}' to the pkg-config call would be an upstreamable fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was trying to do something like that earlier, missed --define-variable. Yeah that sounds reasonable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not test but looks good to me.

@bobby285271
Copy link
Member

bobby285271 commented Mar 15, 2022

Tested again on aaad72a using VM, ibus (ibus-libpinyin) works on gnome-text-editor (gtk4), notejot (gtk4), pantheon.elemenaty-code (gtk3), libsForQt5.kate (qt5) on GNOME & GNOME on Xorg & XFCE. Tested the same thing with fcitx5 (fcitx5-chinese-addons).

When testing ibus-libpinyin on GTK4 apps, the candidate window position does not seem right for me, but I don't see similar issue on fcitx5 (fcitx5-chinese-addons) so maybe this is a ibus specific issue (not sure).

Copy link
Member

@bobby285271 bobby285271 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test result LGTM, nixpkgs-review happy on x86_64-linux

Result of nixpkgs-review pr 164105 run on x86_64-linux 1

4 packages marked as broken and skipped:
  • libsForQt512.kdeplasma-addons
  • libsForQt512.plasma-desktop
  • libsForQt514.kdeplasma-addons
  • libsForQt514.plasma-desktop
3 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
  • tests.nixos-functions.nixosTest-test
54 packages built:
  • adapta-gtk-theme
  • chrome-gnome-shell
  • enlightenment.econnman
  • enlightenment.ecrire
  • enlightenment.efl
  • enlightenment.enlightenment
  • enlightenment.ephoto
  • enlightenment.evisum
  • enlightenment.rage
  • enlightenment.terminology
  • gnome.gnome-control-center
  • gnome.gnome-flashback
  • gnome.gnome-session
  • gnome.gnome-shell
  • gnome.gnome-terminal
  • gnome.gnome-tweaks
  • gnomeExtensions.easyScreenCast
  • gnomeExtensions.gsconnect
  • gnomeExtensions.night-theme-switcher
  • haskellPackages.gi-ibus
  • ibus
  • ibus-engines.anthy
  • ibus-engines.hangul
  • ibus-engines.kkc
  • ibus-engines.libpinyin
  • ibus-engines.libthai
  • ibus-engines.m17n
  • ibus-engines.mozc
  • ibus-engines.rime
  • ibus-engines.table
  • ibus-engines.table-chinese
  • ibus-engines.table-others
  • ibus-engines.typing-booster
  • ibus-engines.typing-booster-unwrapped
  • ibus-engines.uniemoji
  • ibus-qt
  • ibus-with-plugins
  • libsForQt5.kdeplasma-addons (libsForQt515.kdeplasma-addons ,plasma5Packages.kdeplasma-addons)
  • libsForQt5.plasma-desktop (libsForQt515.plasma-desktop ,plasma5Packages.plasma-desktop)
  • mlterm
  • pantheon.elementary-greeter
  • pantheon.elementary-session-settings
  • pantheon.switchboard-plug-keyboard
  • pantheon.switchboard-with-plugs
  • pantheon.wingpanel-applications-menu
  • pantheon.wingpanel-indicator-keyboard
  • pantheon.wingpanel-with-indicators
  • phosh
  • python310Packages.pythonefl
  • python39Packages.pythonefl
  • snippetpixie
  • tests.trivial-builders.references
  • vimix-gtk-themes
  • whitesur-gtk-theme

@jian-lin
Copy link
Contributor

jian-lin commented Mar 15, 2022

tested this patch based on 062a0c5 on gnome-text-editor(gtk4).

I can input Chinese using ibus-rime, but the candidate window is invisible. On gedit(gtk3), it works well.

FWIW, my environment is gnome on x11 with nvidia card.

Screenshot from 2022-03-15 15-19-01

@ncfavier
Copy link
Member Author

Should we merge this and investigate the GTK 4 issues separately? I think the changes in this PR at least make sense, but more work may be needed.

@jtojnar jtojnar merged commit 0f94c5b into NixOS:master Mar 15, 2022
GNOME 42 automation moved this from In progress to Done Mar 15, 2022
@ncfavier ncfavier deleted the ibus branch March 15, 2022 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

ibus: support gtk4
4 participants