input methods: use GTK3_IM_MODULE_FILE#14417
input methods: use GTK3_IM_MODULE_FILE#14417ericsagnes wants to merge 2 commits intoNixOS:masterfrom
Conversation
There was a problem hiding this comment.
Use Environment as a set: Environment.GTK_PATH = "....".
|
If |
|
@abbradar Thanks, I added a commit to remove the systemd unit and use |
There was a problem hiding this comment.
Add preferLocalBuild = true; allowSubstitutes = false; to those -- they look simple enough.
There was a problem hiding this comment.
Thanks, I added a commit fixing this.
|
Nice! LGTM, I'll wait for someone else with more GTK expertise to review it but if I understand everything correctly, this should work. Also we may want to port this to stable although it's somewhat mass-rebuildy. @domenkozar, what do you think? |
|
Oh, one more thing -- don't sweat over |
There was a problem hiding this comment.
Oh, right -- you should mark this as internal = true, I think.
There was a problem hiding this comment.
Neat! Thanks, I added a commit.
|
Looking at previous discussions on the topic (#2450), I think that @lethalman might be interested by this PR. |
|
@ericsagnes Would you mind patching for the other environment variables GTK 3 reads, the same way you have done for |
|
We really need to patch every call to |
|
@ttuegel Result of a quick
Is it fine to patch all these? |
|
Yes, that sounds good! |
|
From cursory glance, some of these may be unnecessary (for example, |
|
Tested using I got a strange behavior: if The error does not occur with |
|
@ttuegel I added a commit patching all the variables cited in the previous comment. |
|
@ericbmerritt Very cool! Maybe we want to try upstreaming this (of course, out of scope of this PR)? |
|
Ouch, sorry, auto-completion. I meant @ericsagnes |
|
@ericsagnes I agree now that the variables which are testing for |
|
@abbradar Good suggestion, I added a ticket upstream.
@ttuegel I can revert that if you think it is better, should I? |
|
@ericbmerritt Thanks! Let's see what upstream thinks. From another look (brief, though) those variables can be actually useful to have GTK3_ variants:
Notice that I haven't included Others are not really important and/or mostly for debugging, I think. Notice that I don't have any knowledge on GTK topics and this is just me reading the patch and concluding what a variable does from its name! |
I think the problem with GTK_EXE_PREFIX as things stand now is that it must be shared with GTK 2. I would prefer to have separate variables; in worst-case scenario, they always take the same value. Otherwise, I agree with you list of variables that need separate prefixes. Thanks for looking into this! |
|
@ttuegel Hm, indeed, this would be more convenient (allows us to separate GTK2 and 3 configuration, enable their support separately etc). So I agree, let's patch |
|
@abbradar I added a commit to remove the patches of the following variables:
So now, the only patched variables are:
|
Thank you! This looks ready to me now.Nikolay. |
abbradar
removed
the
0.kind: enhancement
|
I arranged the commits so the PR is ready to merge. @taku0 Have you found the cause of the |
ericsagnes
changed the title
|
@abbradar we can eventually backport to 16.03, there's no staging for it. But for mass rebuilds, it should first be committed to master and tested for a few weeks. Then open a PR to backport to stable with arguments why it's feasible to do so. |
|
@domenkozar Got it, thanks Let's wait for @taku0's response to ensure it isn't this patch's fault and merge. |
|
It crashes when I try to input to an text field of |
|
@taku0 I would like to check if the same happens on my test machine, can you tell which text field of |
|
|
Seeing the upstream reaction to this patch, I wonder why won't we just patch our default paths into GTK instead? So it would expect IM cache in |
GTK applications won't work outside NixOS. I don't have a problem with that; given upstream's obstructionist attitude, we should drop all GTK packages anyway. That's not going to happen, so I would say: go with the patch we already have. |
A-ha, indeed: I haven't considered that. Thanks for the clarification! |
|
@taku0 Thanks for the detailed steps! I could reproduce the errors in the vm, but it is working fine on my physical test machine. |
|
I will try to test on this weekend. |
This patch the following environment variables logic, enabling to use a `GTK3_` version as an override: - GTK_IM_MODULE_FILE - GTK_THEME - GTK_DATA_PREFIX - GTK_PATH - GTK_IM_MODULE - GTK_EXE_PREFIX Details at NixOS#14417
|
Update from the upstream bug report:
From the bug report:
So shall we merge the PR an keep this patch until upstream fixes the default functionality or try a different approach? |
|
I say let's merge -- while upstream fixes this, we want to have a working solution for our users. I don't think we as distribution maintainers need a better fix (of course, we as (GTK) users may want one, but that's another story). I also propose to move discussion about @taku0's problem to another issue. If noone objects, I'll hit the button in several days (feel free to beat me to it, of course!). (EDIT to clarify: by "GTK users" I meant mainly developers) |
|
Thank you, this commit fixed my UIM. :) |
|
...and broke pasystray. Backtrace shows that pasystray's GTK+3 loads UIM plugin for GTK+2 and gets SIGABRT later. We need to test and fix upgrade path for GTK+3 apps before merging this. |
|
@astsmtl I tried |
|
@astsmtl Wild guess -- maybe you use |
Another option is reverting a91161a partially for |
|
Of course it was pasystray from my profile. But is this a good excuse for breaking? Maybe we can also patch GTK+2 to use GTK2_XXX variables? |
This is a fair point. One way we could fix this issue without breaking existing packages is to patch GTK 2 and 3 to search NIX_PROFILES for an immodules.cache file. I patched Qt 5 this way for its plugins. We could have GTK search each profile for a file EDIT: There's one other option, which upstream alluded to: hardcode the choice of input method at compile time and ship a binary cache with I-Bus as the default. (Realistically, I-Bus is the only mature, modern input method on Linux.) I think we should seriously consider this option. As a distribution, it's our job to be software integrators for our users, i.e. to provide a distribution not a mere binary cache. I think this point is usually lost in Nixpkgs. The "throw all possible options at the user" approach isn't responsible. This is not Gentoo or LFS. |
|
Ideally, the following three GTK+ versions must match:
The This is an unsolved problem of Nix. |
|
@ericsagnes
|
@ttuegel That sounds like a cleaner approach, I made an implementation here. It is working fine with @taku0 thanks for testing, I have no idea about what can be the cause as other gtk apps are working. Does the NIX_PROFILES version work any better? |
Yes, I think that would be preferable. |
|
closing in favor of #14568. |
8 participants
Work in progress, please do not merge yet.This is an implementation of #14019 fix.
@ttuegel Could you review this PR code?
A few personal concerns:
Using a systemd unit to generate the cache files doesn't feel right, is there a better way to do that?solved in the second commitHaving an option (solved by settingi18n.inputMethod.package) just to pass the input method package doesn't feel right.internal = trueNote: