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

WIP gnomeExtensions.text-translator: init at unstable-2020-02-25 #81079

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ericdallo
Copy link
Member

Motivation for this change

Add gnome extension text-translator.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

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; [
Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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!

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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!

Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to work:
image

How can I test it from terminal?

@jtojnar
Copy link
Contributor

jtojnar commented Mar 15, 2020

Also it does not look like it supports GNOME Shell 3.36 so it will become a problem soon.

@ericdallo ericdallo force-pushed the gnome-extensions-text-translator branch from 7bd631e to 01743e3 Compare March 15, 2020 19:06
@ericdallo ericdallo force-pushed the gnome-extensions-text-translator branch from 01743e3 to e9b9442 Compare March 21, 2020 00:48
Copy link
Member

@veprbl veprbl left a 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

@ericdallo ericdallo force-pushed the gnome-extensions-text-translator branch from e9b9442 to 053a22a Compare March 21, 2020 01:04
@jtojnar
Copy link
Contributor

jtojnar commented Mar 21, 2020

You can test GNOME 3.36 on #81626.

@ericdallo ericdallo force-pushed the gnome-extensions-text-translator branch from 053a22a to b459728 Compare March 21, 2020 01:20
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

LGTM

@ericdallo
Copy link
Member Author

Tested on gnome 3.36, It works!

@jtojnar
Copy link
Contributor

jtojnar commented Mar 22, 2020

It is also good idea to have journalctl -xeaf running in a console before enabling the extension to detect any silent breakage.

@jtojnar
Copy link
Contributor

jtojnar commented Mar 22, 2020

Also do you have a link to the issue about the missing license?

@ericdallo
Copy link
Member Author

Yeah, I opened it here @jtojnar

@ericdallo
Copy link
Member Author

ericdallo commented Mar 22, 2020

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 Clutter 😔
image

I also found this on journalctl
image

Should I open an issue to the upstream? It already has this issue for gnome 3.36

@jtojnar
Copy link
Contributor

jtojnar commented Mar 22, 2020

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.

@ericdallo ericdallo force-pushed the gnome-extensions-text-translator branch from b459728 to d27b247 Compare March 22, 2020 14:47
@ericdallo
Copy link
Member Author

Sorry @jtojnar, I didn't see this comment.
I added the patch for clutter here, but still has the same error message on Gnome Extensions.

@ericdallo ericdallo force-pushed the gnome-extensions-text-translator branch from d27b247 to 8087760 Compare March 22, 2020 15:01
@jtojnar
Copy link
Contributor

jtojnar commented Mar 22, 2020

Could you please paste the traces here as text? It will be easier for me to debug it.

@ericdallo
Copy link
Member Author

Sorry 😅
Gnome Extensions:

The settings of extension text_translator@awamper.gmail.com had an error:

Error: Requiring Clutter, version none: Typelib file for namespace 'Clutter' (any version) not found

Stack trace:

@/home/greg/.local/share/gnome->shell/extensions/text_translator@awamper.gmail.com/utils.js:10:17
@/home/greg/.local/share/gnome->shell/extensions/text_translator@awamper.gmail.com/prefs.js:8:15
get prefsModule@resource:///org/gnome/shell/extensionPrefs/main.js:733:13
_showPrefs@resource:///org/gnome/shell/extensionPrefs/main.js:174:13
openPrefs@resource:///org/gnome/shell/extensionPrefs/main.js:159:24
_init/<@resource:///org/gnome/shell/extensionPrefs/main.js:600:62
main@resource:///org/gnome/shell/extensionPrefs/main.js:762:23
@<main>:1:48

@jtojnar
Copy link
Contributor

jtojnar commented Mar 22, 2020

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

if you're adding stuff to gnome-shell, you'll use clutter
if you're adding preferences, you'll use gtk

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

@jtojnar
Copy link
Contributor

jtojnar commented Mar 22, 2020

Otherwise the GIRepository.Repository.prepend_search_path would need to all the extension entry points that load Clutter transitively. prefs.js and extension.js are all entry points IIRC, and extension.js will not need it since it gets Clutter from GNOME Shell.

@ericdallo
Copy link
Member Author

Thanks for your great explanation @jtojnar, I'll change it!

@ericdallo ericdallo force-pushed the gnome-extensions-text-translator branch from 8087760 to 453e141 Compare March 22, 2020 15:30
GIRepository.Repository.prepend_search_path("/usr/lib/" + path);
}
);
+ GIRepository.Repository.prepend_search_path('@clutter@');
Copy link
Contributor

@jtojnar jtojnar Mar 22, 2020

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping.

Copy link
Member Author

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.

@ericdallo
Copy link
Member Author

It seems to work now the preferences @jtojnar, but the enable extension button is disabled...
I think it's related to the journalctl error:

Mar 22 13:16:00 gregnix-note .gnome-shell-wr[2084]: JS ERROR: Extension text_translator@awamper.gmail.com: TypeError: this._player is null
                                                    GoogleTTS@/run/current-system/sw/share/gnome-shell/extensions/text_translator@awamper.gmail.com/google_tts.js:12:9
                                                    _init@/run/current-system/sw/share/gnome-shell/extensions/text_translator@awamper.gmail.com/translator_dialog.js:333:28
                                                    TranslatorExtension@/run/current-system/sw/share/gnome-shell/extensions/text_translator@awamper.gmail.com/extension.js:287:24
                                                    enable@/run/current-system/sw/share/gnome-shell/extensions/text_translator@awamper.gmail.com/extension.js:1079:18
                                                    _callExtensionEnable@resource:///org/gnome/shell/ui/extensionSystem.js:160:32
                                                    loadExtension@resource:///org/gnome/shell/ui/extensionSystem.js:311:26
                                                    _loadExtensions/<@resource:///org/gnome/shell/ui/extensionSystem.js:536:18
                                                    collectFromDatadirs@resource:///org/gnome/shell/misc/fileUtils.js:27:17
                                                    _loadExtensions@resource:///org/gnome/shell/ui/extensionSystem.js:515:19
                                                    _enableAllExtensions@resource:///org/gnome/shell/ui/extensionSystem.js:545:18
                                                    _sessionUpdated@resource:///org/gnome/shell/ui/extensionSystem.js:576:18
                                                    init@resource:///org/gnome/shell/ui/extensionSystem.js:54:14
                                                    _initializeUI@resource:///org/gnome/shell/ui/main.js:244:22
                                                    start@resource:///org/gnome/shell/ui/main.js:138:5
                                                    @<main>:1:47

@jtojnar
Copy link
Contributor

jtojnar commented Mar 23, 2020

Looking at the source code, it might be caused by missing playbin element. According to the GStreamer docs, that comes from gst_all_1.gst-plugins-base package. Playing with gjs looks like that could be the case:

$ 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]

@ericdallo ericdallo force-pushed the gnome-extensions-text-translator branch from 453e141 to 58f30ca Compare March 23, 2020 01:32
@ericdallo
Copy link
Member Author

It works @jtojnar!
image

I got some warnings on journalctl and already sent it to upstream

Thanks for you patience and help!

@ericdallo ericdallo force-pushed the gnome-extensions-text-translator branch from 58f30ca to 87c5830 Compare March 23, 2020 12:21
@ericdallo ericdallo requested a review from jtojnar March 23, 2020 12:21
Comment on lines +41 to +58
--- 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) {
Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

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 see, just temporary till merging.
Ok, I'll work on that!

@veprbl veprbl changed the title gnomeExtensions.text-translator: init at unstable-2020-02-25 WIP gnomeExtensions.text-translator: init at unstable-2020-02-25 May 25, 2020
@ryantm ryantm marked this pull request as draft October 23, 2020 03:02
@stale
Copy link

stale bot commented Jun 4, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 4, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants