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

extension: add a desktop entry #297

Merged
merged 23 commits into from
Jan 13, 2023
Merged

Conversation

pesader
Copy link
Contributor

@pesader pesader commented Jan 8, 2023

Add a desktop file so ddterm is displayed more nicely (with an icon and a proper name) in the activities view and application menu. Install the file to the user's local applications directory when the extension is enabled and remove it when the extension is disabled.

Here's a demo of it working:

Kooha-2023-01-08-15-56-21.mp4

Closes #123.

Add a desktop file so ddterm is displayed more nicely (with an icon and
a proper name) in the activities view and application menu. Install the
file to the user's local applications directory when the extension is
enabled and remove it when the extension is disabled.
@pesader
Copy link
Contributor Author

pesader commented Jan 8, 2023

I ran the linter locally and it seems that the only files that are throwing errors are rxjs.js and handlebars.js, which I did not edit.

@amezin
Copy link
Member

amezin commented Jan 10, 2023

I ran the linter locally and it seems that the only files that are throwing errors are rxjs.js and handlebars.js, which I did not edit.

It shouldn't lint rxjs and handlebars at all - they should be ignored. Try running make lint.

Here eslint fails on extension.js: https://github.com/ddterm/gnome-shell-extension-ddterm/actions/runs/3868698513/jobs/6594312408#step:7:12

And, by the way, ddterm still supports Ubuntu 20.04 with rather old versions of GNOME Shell and gjs, so some language features may be missing (though I'm not sure if eslint rejects the initializer here correctly).

@amezin
Copy link
Member

amezin commented Jan 10, 2023

I wonder why the error doesn't generate an annotation on the pull request though...

ddterm/shell/extension.js Outdated Show resolved Hide resolved
Since ddterm provides an "Activate" method via dbus, specify that in its
desktop entry. In accordance with the Freedesktop Spec, the Exec key is
not removed to keep compatibility with implementations that do not
handle the DBusActivatable key properly.
Make all properties and methods of the DesktopEntry class non-static,
so it can be instantiated on enable() and destroyed on disable()
If caught exception happens because desktop file already exists, do
nothing. Otherwise, throw the caught exception.
@pesader
Copy link
Contributor Author

pesader commented Jan 12, 2023

Thank you for taking the time to review this PR and (especially) for pointing me to good references and learning resources. I've made the changes you requested, but it is now failing a ton of tests on every OS.

I'll try to figure out what's going on.

@amezin
Copy link
Member

amezin commented Jan 12, 2023

There is a lot of noise, but I believe the root cause is that the extension fails to load:

[  531.628757] gnome-shell[79]: JS ERROR: Extension ddterm@amezin.github.com: Gio.IOErrorEnum: Error opening file “/home/gnomeshell/.local/share/applications/com.github.amezin.ddterm.desktop”: No such file or directory
install@/home/gnomeshell/.local/share/gnome-shell/extensions/ddterm@amezin.github.com/ddterm/shell/extension.js:166:16
enable@/home/gnomeshell/.local/share/gnome-shell/extensions/ddterm@amezin.github.com/ddterm/shell/extension.js:256:19
enable@/home/gnomeshell/.local/share/gnome-shell/extensions/ddterm@amezin.github.com/extension.js:32:10
_callExtensionEnable@resource:///org/gnome/shell/ui/extensionSystem.js:182:32
_onEnabledExtensionsChanged/<@resource:///org/gnome/shell/ui/extensionSystem.js:527:35
_onEnabledExtensionsChanged@resource:///org/gnome/shell/ui/extensionSystem.js:527:14
createCheckedMethod/<@resource:///org/gnome/gjs/modules/core/overrides/Gio.js:533:46
enableExtension@resource:///org/gnome/shell/ui/extensionSystem.js:209:29
EnableExtension@resource:///org/gnome/shell/ui/shellDBus.js:447:38
_handleMethodCall@resource:///org/gnome/gjs/modules/core/overrides/Gio.js:310:38
_wrapJSObject/<@resource:///org/gnome/gjs/modules/core/overrides/Gio.js:387:34

https://github.com/ddterm/gnome-shell-extension-ddterm/actions/runs/3905791923/jobs/6673238419#step:13:11571

Maybe ~/.local/share/applications doesn't exist?

Every concurrent test session spawns a new container, so it shouldn't be a race between file creation/deletion/existence check.

Make all properties and methods of the DesktopEntry class non-static,
so it can be instantiated on enable() and destroyed on disable()
If caught exception happens because desktop file already exists, do
nothing. Otherwise, throw the caught exception.
ddterm/shell/extension.js Outdated Show resolved Hide resolved
ddterm/shell/extension.js Outdated Show resolved Hide resolved
@pesader
Copy link
Contributor Author

pesader commented Jan 12, 2023

It now ensures the user applications directory exists before it installs the desktop file there. I'll now address the case that the extension is installed system-wide.

@pesader
Copy link
Contributor Author

pesader commented Jan 12, 2023

All tests passed 🙌🏻

Maybe ~/.local/share/applications doesn't exist?

Your intuition was correct!

Copy link
Member

@amezin amezin left a comment

Choose a reason for hiding this comment

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

Sorry that I didn't respond with all questions at once - I didn't notice multiple things at the first sight

ddterm/shell/extension.js Outdated Show resolved Hide resolved
ddterm/com.github.amezin.ddterm.desktop Outdated Show resolved Hide resolved
ddterm/com.github.amezin.ddterm.desktop Outdated Show resolved Hide resolved
ddterm/shell/extension.js Outdated Show resolved Hide resolved
ddterm/shell/extension.js Outdated Show resolved Hide resolved
ddterm/shell/extension.js Show resolved Hide resolved
@pesader
Copy link
Contributor Author

pesader commented Jan 13, 2023

There seems to be a permission error now, which I don't get.

When using make_directory_with_parents() instead of mkdir_with_parents() the file permission was 644 (everyone allowed to read, user allowed to read an write), but there's a Permission Error when a test attempts to open the desktop file in the CI now.

ddterm/shell/extension.js Outdated Show resolved Hide resolved
@amezin
Copy link
Member

amezin commented Jan 13, 2023

Thanks for your patience and fixing all issues

@amezin amezin merged commit 7ee4db0 into ddterm:master Jan 13, 2023
amezin added a commit that referenced this pull request Jan 13, 2023
`terminal` isn't a standard icon name. Maybe it's available in
some icon themes, but not in the default GNOME one.

The correct name is `utilities-terminal`:

https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html

(which is also used by the panel icon)

Fix for #297
@pesader
Copy link
Contributor Author

pesader commented Jan 13, 2023

Thanks for your patience teaching me how to fix them! Cheers

@amezin
Copy link
Member

amezin commented Jan 13, 2023

If you wish to work on D-Bus activation - you would really help a lot if you implemented it. Multiple features have it as a prerequisite.

@pesader
Copy link
Contributor Author

pesader commented Jan 13, 2023

Sure would like to :)
Is it issue #79 or should I open another issue to discuss that?

@amezin
Copy link
Member

amezin commented Jan 14, 2023

@pesader no, that issue, even though related, isn't the correct one.

I've opened a dedicated issue for D-Bus activation: #304

@pesader pesader mentioned this pull request Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an icon to ddterm
2 participants