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

Fix Get/Set-Clipboard on Linux/Wayland with wl-clipboard #24049

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

helpimnotdrowning
Copy link

@helpimnotdrowning helpimnotdrowning commented Jul 13, 2024

PR Summary

Uses wl-copy/wl-paste from wl-clipboard if a Wayland session is in use ($env:XDG_SESSION_TYPE is wayland), otherwise fallback to previous default xclip

PR Context

Fixes #17561
Supersedes #18227

PR Checklist

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

My overall concern with this approach is that I could not find XDG_SESSION_TYPE in the FDO specs (https://www.freedesktop.org/wiki/) anywhere (or even discussion of adding it). I also found that there's some inconsistency with the way this is set on some systems (see https://unix.stackexchange.com/questions/202891/how-to-know-whether-wayland-or-x11-is-being-used, and others).

I do see the need, but i think perhaps we should not squat the XDG space but do something for PowerShell specifically. If we take this approach, then we can likely have the same solution on all platforms to support the various clipboards in the market.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 22, 2024
@helpimnotdrowning
Copy link
Author

helpimnotdrowning commented Jul 22, 2024

$env:XDG_SESSION_TYPE seems to be defined here: https://www.freedesktop.org/software/systemd/man/latest/pam_systemd.html#type= & https://www.freedesktop.org/software/systemd/man/latest/sd_session_get_type.html#, deep in the systemd man pages they host. I'm not personally sure if this means that they are "official" or just "there".

We could also instead check for the presence of $env:WAYLAND_DISPLAY, which (should hopefully) always be set.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 22, 2024
@JamesWTruher
Copy link
Member

JamesWTruher commented Jul 23, 2024

$env:XDG_SESSION_TYPE seems to be defined here: https://www.freedesktop.org/software/systemd/man/latest/pam_systemd.html#type= & https://www.freedesktop.org/software/systemd/man/latest/sd_session_get_type.html#, deep in the systemd man pages they host. I'm not personally sure if this means that they are "official" or just "there".

We could also instead check for the presence of $env:WAYLAND_DISPLAY, which (should hopefully) always be set.

pam_systemd

sd_session_is_active

yah - that was more about systemd (and it doesn't look like much it's going to be incorporated into the mainline specs)

I was thinking that a flexible approach would be along the lines of a PowerShell variable, which could provide for more subtle behaviors. Please don't focus on the names, just the flexibility of the approach.

$PSClipboardActions = @{
     Clip = @{
        Command = "your clipper here"
        Arguments = "-l","-otherparameter"
     }
     Paste = @{
        Command = "your paster here"
        Arguments = "-param1","-param2"
     }
}

There's no real reason it has to be an environment variable either, as this sort of thing is happening in an interactive session. Also, this approach would work for all platforms.

Since freedesktop doesn't seem to be moving forward in this area, we shouldn't necessarily be bound by it. It seemed to me that the systemd was an initial foray into the arena (and seems to be inconsistently implemented).

I suppose the above could be extended to be a scriptblock to evaluate at runtime which clipboard stuff you want, which might enable clip-stacks and even more flexibility. Encapsulating all that in a simple string might be pretty tricky :)

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Jul 30, 2024
@SteveL-MSFT SteveL-MSFT added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 5, 2024
@daxian-dbw daxian-dbw added WG-Cmdlets general cmdlet issues WG-NeedsReview Needs a review by the labeled Working Group and removed Review - Needed The PR is being reviewed labels Aug 5, 2024
@SteveL-MSFT
Copy link
Member

The @PowerShell/wg-powershell-cmdlets discussed this, we see 3 areas of interest pertaining to this issue:

  • Detection of Wayland should use the WAYLAND_DISPLAY env var rather than relying on the XDG_SESSION_TYPE which is owned by OpenDesktop where Wayland currently doesn't play a role
  • We agreed that to provide flexibility and be more future proof, it makes sense to expose a PowerShell variable for cut and paste as Jim proposes
  • Then it makes sense to simplify the clipboard cmdlets to use this PowerShell variable and thus the detection code (including Wayland) for setting defaults to the PowerShell variable should reside in the engine which would also allow other tools to use these variables

We also defer to the Interactive WG to discuss this proposal

@SteveL-MSFT SteveL-MSFT added WG-Interactive-Console the console experience and removed WG-Cmdlets general cmdlet issues labels Aug 21, 2024
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

WG feedback needs to be adddressed

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 17, 2024
@helpimnotdrowning
Copy link
Author

Hi, I'm currently trying my best to see what code should go where, so progress is slow (along with more sudden busyness)

On Windows, the clipboard is set using Windows-specific code, not a command like clip. How should I handle this?
On Windows, the clipboard is handled internally, not through a command like clip.exe. I had a few ideas on how I should handle it, but none of these seem great to me:

  1. A third field in the $PSClipboardAction variable, which is checked. If true, Clip and Paste are ignored
$PSClipboardActions = @{
	Clip = @{ UseInternalWindowsThing = $true }
	Paste = @{ UseInternalWindowsThing = $true }
}
  1. New class that will specifically hold all these options in constructors like ClipboardActions(string), ClipboardActions(string, string[]), ClipboardActions(bool), where $PSClipboardActions is modified in-shell like
# one string
$PSClipboardActions = @{
	Clip = [ClipboardActions]::new("wl-copy"); # <-- runs "<input> | wl-copy"
	Paste = [ClipboardActions]::new("wl-paste");
}
# string, string[]
$PSClipboardActions = @{
	Clip = [ClipboardActions]::new("xclip", @("-selection", "clipboard", "-in")); # <-- runs "<input> | xclip -selection clipboard -in"
	Paste = [ClipboardActions]::new("xclip", @("-selection", "clipboard", "-out"));
}
# bool ?
$PSClipboardActions = @{
	Clip = [ClipboardActions]::new($true); # <-- uses the Windows stuff already in Microsoft.PowerShell.Commands.Internal.Clipboard
	Paste = [ClipboardActions]::new($true);
}

but I feel like this reads poorly (in my uninformed opinion). Any ideas?

@theJasonHelmick
Copy link
Collaborator

The WG Interactive agrees with the @SteveL-MSFT plan.

@theJasonHelmick theJasonHelmick removed the WG-NeedsReview Needs a review by the labeled Working Group label Sep 18, 2024
{
foreach (Object arg in args)
{
startInfo.ArgumentList.Add(Convert.ToString(arg));
Copy link
Author

Choose a reason for hiding this comment

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

Args are now passed with an array, is this a good way to put it into startInfo.ArgumentList?

return _internalClipboard ?? string.Empty;
}
// need errors for null clipboardActions
// need errors for null clipboardActions["Paste"]
Copy link
Author

Choose a reason for hiding this comment

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

Need to raise exceptions for a null clipboardActions ($PSClipboardActions) and null clipboardActions["Paste"], currently shows a somewhat cryptic Object reference not set to an instance of an object. (NRE), correct?

Also need guidence on language for exception text (see line 89)

{
tool = "wl-paste";
throw new PlatformNotSupportedException("1REPLACEME Can't run Windows clipboard methods on non-Windows platforms!");
Copy link
Author

Choose a reason for hiding this comment

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

Exception language guidence?

_clipboardSupported = false;
return string.Empty;
// write-warning "PSClipboardActions.Paste.Command is not set" ???
return _internalClipboard ?? string.Empty;
Copy link
Author

Choose a reason for hiding this comment

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

Best I could do to emulate the previous behaviour for the tests, but tests should now set $PSClipboardActions to $null to get here (also forgot to update the tests again...)

}
else
{
_clipboardSupported = false;
return string.Empty;
// write-warning "PSClipboardActions.Paste.Command is not set" ???
Copy link
Author

Choose a reason for hiding this comment

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

Should this write a warning or something if Command isn't set?

// $PSClipboardActions
Hashtable clipboardActions = null;

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
Copy link
Author

Choose a reason for hiding this comment

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

I really don't like that I stuffed all the platform logic in this file specifically, but I couldn't find a better place; is this good?

{
clipboardActions = new Hashtable {
{ "Clip", new Hashtable {
{ "UseWindowsClipboard", true }
Copy link
Author

@helpimnotdrowning helpimnotdrowning Sep 20, 2024

Choose a reason for hiding this comment

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

Not a fan of having to use Hashtables (instead of dicts) because Clip/Pastes values can be strings, string[]s, or bools, and they all have to be manually re-cast in Clipboard.cs. Is there a better way to do this, or is a perfect solution going to require a seperate class (see point 2 in #24049 (comment) )?

@@ -123,6 123,9 @@
<data name="PSHOMEDescription" xml:space="preserve">
<value>Parent folder of the host application of the current runspace</value>
</data>
<data name="PSClipboardActions" xml:space="preserve">
<value>replaceme Commands used to control Get-Clipboard and Set-Clipboard commands</value>
Copy link
Author

Choose a reason for hiding this comment

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

Language guidence?

@helpimnotdrowning
Copy link
Author

Also: this is untested on Windows since I don't want to go through the hassle of installing VS2019, but setting Clip/Paste to @{UseWindowsClipboard=$true} does show the correct exception text, so it's probably okay

Copy link
Contributor

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review - Needed The PR is being reviewed WG-Interactive-Console the console experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clipboard commands don't work on Linux Wayland
6 participants