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
WIP gnomeExtensions.text-translator: init at unstable-2020-02-25 #81079
base: master
Are you sure you want to change the base?
Conversation
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/129 |
}) | ||
]; | ||
|
||
nativeBuildInputs = with pkgs; [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these in nativeBuildInputs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I change to buildInputs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, run-time dependencies should go to buildInputs
, though for extensions that does not work. See https://nixos.org/nixpkgs/manual/#ssec-gnome-common-issues-unwrappable-package for a solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jtojnar, I'll fix it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtojnar, I think this package doesn't need this buildInputs, I removed them and built the derivation again in qemu and it seems to work correctly, should I just remove these inputs? The extension doesn't seems to need it, right?
I just added because I thought it was really necessary checking here, am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Searching the source code, it only seems to use Clutter
, not ClutterGst
or GtkClutter
, so the README is probably incorrect. And Clutter
is provided by Mutter, so it is already available to GNOME Shell. That might explain why you do not need to add it explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you might still need to add ${mutter}/lib/mutter-5
${gnome3.mutter}/lib/mutter-5
or ${clutter}/lib/girepository-1.0
when opening the preferences from GNOME Tweaks, which does not run in GNOME Shell context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, got it!
Removed these dependencies, thanks @jtojnar!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does opening preferences from GNOME Tweaks work then? Might want to run them from Terminal and check the console output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it does not look like it supports GNOME Shell 3.36 so it will become a problem soon. |
7bd631e
to
01743e3
Compare
01743e3
to
e9b9442
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple more comments, sorry for not noticing before
e9b9442
to
053a22a
Compare
You can test GNOME 3.36 on #81626. |
053a22a
to
b459728
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Tested on gnome 3.36, It works! |
It is also good idea to have |
Also do you have a link to the issue about the missing license? |
I think I was on gnome 3.34thinking it was 3.36 when I tested @jtojnar, now I'm really on gnome 3.36 and the extension is with an error requiring I also found this on Should I open an issue to the upstream? It already has this issue for gnome 3.36 |
Our 3.34 package had clutter unnecessarily in dependencies so it probably leaked to the extension. With 3.36, we no longer do that since Mutter has used its own internal Clutter fork for ages but that one probably might not be available to Shell extensions. So it will probably require the gi repository hack as described above. |
b459728
to
d27b247
Compare
Sorry @jtojnar, I didn't see this comment. |
d27b247
to
8087760
Compare
Could you please paste the traces here as text? It will be easier for me to debug it. |
Sorry 😅
|
Looks like the issue is that extension imports clutter in the utils, and then imports utils in prefs.js, even though the functions using clutter might not be used in prefs. From GNOME IRC
so we might want to patch Utils to import clutter only in the function that uses it: Move https://github.com/gufoe/text-translator/blob/2992bb34574c366f602056f0d3eadb48ebc988b3/utils.js#L10 to https://github.com/gufoe/text-translator/blob/2992bb34574c366f602056f0d3eadb48ebc988b3/utils.js#L130 |
Otherwise the |
Thanks for your great explanation @jtojnar, I'll change it! |
8087760
to
453e141
Compare
GIRepository.Repository.prepend_search_path("/usr/lib/" + path); | ||
} | ||
); | ||
+ GIRepository.Repository.prepend_search_path('@clutter@'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the comments, extension.js
does not need path to clutter, it will get it from GNOME Shell/Mutter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm gonna fix it.
It seems to work now the preferences @jtojnar, but the enable extension button is disabled...
|
Looking at the source code, it might be caused by missing $ nix-shell -p gobject-introspection -p gst_all_1.gstreamer -p gst_all_1.gst-plugins-base -p gjs --run "gjs -c \"print(imports.gi.Gst.init(null)); print(imports.gi.Gst.ElementFactory.make('playbin', 'player'))\""
[object instance wrapper GType:GstPlayBin jsobj@0x155c07588970 native@0x21f6510]
$ nix-shell -p gobject-introspection -p gst_all_1.gstreamer -p gjs --run "gjs -c \"print(imports.gi.Gst.init(null)); print(imports.gi.Gst.ElementFactory.make('playbin', 'player'))\""
null Could you try another registry hack: https://github.com/NixOS/nixpkgs/pull/57619/files This should be the JavaScript equivalent: $ basePlugins=$(nix-build '<nixpkgs>' -A gst_all_1.gst-plugins-base --no-out-link)
$ nix-shell -p gobject-introspection -p gst_all_1.gstreamer -p gjs --run "gjs -c \"print(imports.gi.Gst.init(null)); imports.gi.Gst.Registry.get().scan_path('$basePlugins/lib/gstreamer-1.0'); print(imports.gi.Gst.ElementFactory.make('playbin', 'player'))\""
[object instance wrapper GType:GstPlayBin jsobj@0x99cf9488a00 native@0xc31c70] |
453e141
to
58f30ca
Compare
It works @jtojnar! I got some warnings on Thanks for you patience and help! |
pkgs/desktops/gnome-3/extensions/text-translator/fix-deps.patch
Outdated
Show resolved
Hide resolved
58f30ca
to
87c5830
Compare
--- a/utils.js | ||
+++ b/utils.js | ||
@@ -7,7 +7,6 @@ const Gio = imports.gi.Gio; | ||
const GLib = imports.gi.GLib; | ||
const ExtensionUtils = imports.misc.extensionUtils; | ||
const Soup = imports.gi.Soup; | ||
-const Clutter = imports.gi.Clutter; | ||
|
||
var _httpSession = new Soup.SessionAsync(); | ||
Soup.Session.prototype.add_feature.call( | ||
@@ -127,6 +126,7 @@ function get_files_in_dir(path) { | ||
} | ||
|
||
function get_unichar(keyval) { | ||
+ const Clutter = imports.gi.Clutter; | ||
let ch = Clutter.keysym_to_unicode(keyval); | ||
|
||
if (ch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be a separate PR that is upstreamed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean a separate patch file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do you mean that upstream should open a PR for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both. I meant opening upstream PR with this part (+ fixing the list of dependencies in readme) and then using fetchpatch
to use it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about this fetchpatch
, it fetches a patch from someplace?
I thought the upstream PR would change only the utils.js
file as we did it on this patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would have two patches:
- fix-deps.patch would remain nin nixpkgs repo
- the move of the import would be PR'd upstream and we would fetch the patch from there using fetchpatch until it gets merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, just temporary till merging.
Ok, I'll work on that!
I marked this as stale due to inactivity. → More info |
Motivation for this change
Add gnome extension
text-translator
.Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)