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

Cli support for general wayland environment #1322

Merged
merged 18 commits into from
Jun 16, 2023

Conversation

JingMatrix
Copy link
Contributor

This commit use wl-clipboard and ydotool to support general wayland environment, such as sway.

Use wl-clipboard for supports on general wayland environment
Use ydotool for general wayland environment
This allows gsconnect-preferences to be excuted when it is symbolically linked to another path.
Use `wtype` for unicode input
Dbus skips the activations of wl-clipboard component due to the lose of
environment variables.
Relying on environment variables for the determination of gnome-shell
is not stable when run as a d-bus service.
@JingMatrix
Copy link
Contributor Author

Please tell me if you have intention to extend this plugin to non-gnome environment, @ferdnyc @andyholmes .
If so, I can add a page to the project wiki to show how it actually works.

@andyholmes
Copy link
Collaborator

I don't have much of an opinion on this. GSConnect was intended to be specifically for GNOME Shell, but if another maintainer wants to give it a 👍 I won't stand in the way 🙂

@daniellandau
Copy link
Member

I have limited time for maintenance, so if making it more generic comes with associated maintenance effort on an ongoing basis I'm all for it. I'll read through the code hopefully soon.

@andyholmes andyholmes added the needs review A contribution that needs code review label Aug 5, 2022
Copy link
Collaborator

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

Sorry this sat around for so long. I don't see any reason not to merge at this point, so a few fixups and I will accept this.

src/service/components/clipboard.js Outdated Show resolved Hide resolved
src/service/components/clipboard.js Outdated Show resolved Hide resolved
src/service/plugins/presenter.js Outdated Show resolved Hide resolved
@andyholmes andyholmes added enhancement A request for a feature or additional functionality and removed needs review A contribution that needs code review labels Jun 15, 2023
@andyholmes
Copy link
Collaborator

Oh, just one more thing: add the SPDX comments to ydotool.js and wl_clipboard.js like so:

// SPDX-FileCopyrightText: GSConnect Developers https://github.com/GSConnect
//
// SPDX-License-Identifier: GPL-2.0-or-later

@JingMatrix
Copy link
Contributor Author

JingMatrix commented Jun 15, 2023

Thanks for reviewing.
Unfortunately, I have forgotten most of the logic of gsconnect, though I am using this pull request for the whole year.
It seems after merging the main branch, remote mousepad isn't working. I need some time to debug it.
Also, I should add a wiki page to explain the usage.

@andyholmes
Copy link
Collaborator

Okay, no problem. I don't expect many folks will start using this for a little while, at least.

If you can get it working for you, we can merge and you can be "codeowner" for that module, if you like.

@JingMatrix
Copy link
Contributor Author

JingMatrix commented Jun 15, 2023

I test it on my machine with both sway and gnome-shell sessions, and there is no problem. (It turned out that I forgot to start ydotoold in sway.)
I would be happy to maintain this module.

Notes for users

When gsconnect is not running as a Gnome extension, for example, running under sway,

  1. You might still need to install the gnome-shell package to provide the Gvc-1.0.typelib file of the gnome module libgnome-volume-control.
  2. Install ydotool and start the service: systemctl status --user ydotool.service
  3. Install wl-clipboard and wl-type

@JingMatrix JingMatrix force-pushed the cli-support branch 2 times, most recently from 9008dfd to 171b52d Compare June 16, 2023 00:53
@JingMatrix
Copy link
Contributor Author

I found that the env GNOME_SETUP_DISPLAY is specially set by gnome, which can thus be used to detect gnome.

Now this pull request should pass all tests without problems, you can merge it.

P.S. it might be interesting to write a test for non-gnome envirment, maybe in the future.

The environment variable is only set by GNOME
@andyholmes
Copy link
Collaborator

P.S. it might be interesting to write a test for non-gnome envirment, maybe in the future.

Definitely, I saw somewhere I think Google Chrome or Chromium had a Wayland version of xvfb built with python or something. I never had time to really get into it though.

@andyholmes andyholmes enabled auto-merge (rebase) June 16, 2023 04:19
@andyholmes andyholmes merged commit 029fa9e into GSConnect:main Jun 16, 2023
2 checks passed
andyholmes pushed a commit that referenced this pull request Jun 16, 2023
Thanks to the review in #1322
@JingMatrix JingMatrix deleted the cli-support branch June 16, 2023 07:45
@JingMatrix
Copy link
Contributor Author

As a note, I add the wiki page of its usage: CLI usage without Gnome.

It will be helpful to put some instructions described there into the meson build process.

@andyholmes
Copy link
Collaborator

That's great, thanks for writing all that up!

@pobrn
Copy link

pobrn commented Dec 4, 2023

This PR introduces a check that uses imports to determine, as far as I understand, if it's running inside gnome shell. What was the problem with that? And when it was replaced, why was it replaced with a check for the GNOME_SETUP_DISPLAY environmental variable? Why wasn't something like GLib.getenv('XDG_CURRENT_DESKTOP') === 'GNOME' || GLib.getenv('XDG_SESSION_DESKTOP') === 'gnome' enough? The only clue I could find is in 9739f97 that states

Relying on environment variables for the determination of gnome-shell is not stable when run as a d-bus service.

So I'm guessing the concern is that checks like the above will break gsconnect if it's run inside a gnome session but not as a gnome shell extension? Because if so, then I believe the current check has the same weakness.

As I mentioned in #1706 (comment) now that gsconnect checks GNOME_SETUP_DISPLAY, some features are broken on all configurations that have no ydotool, and running x11 or running wayland without xwayland.

@JingMatrix
Copy link
Contributor Author

Given what said by @pobrn , it is better not to use environment variable for detecting the 'Gnome'.
I was for using imports to detect Gnome, @andyholmes changed it out of the reason of simplicity (I guess).
Hence, it is favorable to revert the changes regarding the environment variable.
I will do it soon to fix #1706.

@andyholmes
Copy link
Collaborator

FWIW, this was my suggestion:

This won't work in the service, because it's runs outside of the gnome-shell process (meaning it will always throw an error). However, I think this would work:

const HAVE_GNOME = GLib.getenv('XDG_SESSION_TYPE') === 'gnome'

You can put this in src/service/__init__.js as globalThis.HAVE_GNOME if you want, or leave it here.

@JingMatrix
Copy link
Contributor Author

@andyholmes Thanks for mentioning it, I totally forgot it. So I guess we may use the existence of the directory /usr/share/cinnamon/js/ui/* as a detector.

Or, alternatively, is there a way to communicate bewteen the extention.js and daemon.js?
If so, we can set HAVE_GNOME in extension.js.

@JingMatrix
Copy link
Contributor Author

I propose to use imports.gi.Shell as a heuristic for setting HAVE_GNOME.
See my pull-request for details: #1715 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A request for a feature or additional functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants