-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
base: master
Are you sure you want to change the base?
Fix Get
/Set-Clipboard
on Linux/Wayland with wl-clipboard
#24049
Conversation
93f83fa
to
c656e6b
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
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.
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.
We could also instead check for the presence of |
yah - that was more about 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 :) |
The @PowerShell/wg-powershell-cmdlets discussed this, we see 3 areas of interest pertaining to this issue:
We also defer to the Interactive WG to discuss this proposal |
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.
WG feedback needs to be adddressed
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 handled internally, not through a command like
$PSClipboardActions = @{
Clip = @{ UseInternalWindowsThing = $true }
Paste = @{ UseInternalWindowsThing = $true }
}
# 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? |
The WG Interactive agrees with the @SteveL-MSFT plan. |
{ | ||
foreach (Object arg in args) | ||
{ | ||
startInfo.ArgumentList.Add(Convert.ToString(arg)); |
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.
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"] |
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.
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!"); |
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.
Exception language guidence?
_clipboardSupported = false; | ||
return string.Empty; | ||
// write-warning "PSClipboardActions.Paste.Command is not set" ??? | ||
return _internalClipboard ?? string.Empty; |
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.
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" ??? |
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 this write a warning or something if Command
isn't set?
// $PSClipboardActions | ||
Hashtable clipboardActions = null; | ||
|
||
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) |
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 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 } |
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.
Not a fan of having to use Hashtable
s (instead of dicts) because Clip
/Paste
s 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> |
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.
Language guidence?
Also: this is untested on Windows since I don't want to go through the hassle of installing VS2019, but setting |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
sorry to poke, but any update on the review process? @SteveL-MSFT @JamesWTruher |
PR Summary
Uses
wl-copy
/wl-paste
from wl-clipboard if a Wayland session is in use ($env:XDG_SESSION_TYPE
iswayland
), otherwise fallback to previous defaultxclip
PR Context
Fixes #17561
Supersedes #18227
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).