[Advanced Paste] Repeatedly pasting plain text as JSON produces inconsistent results
Summary of the Pull Request
- This PR fixes the overwriting of the formatted text again and again.
PR Checklist
- [x] Closes: #33944
- [x] Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
- [x] Tests: Added/updated and all pass
- [x] Localization: All end user facing strings can be localized
- [x] Dev docs: Added/updated
- [x] New binaries: Added on the required places
- [x] JSON for signing for new binaries
- [x] WXS for installer for new binaries and localization folder
- [x] YML for CI pipeline for new test projects
- [x] YML for signed pipeline
- [x] Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx
Detailed Description of the Pull Request / Additional comments
- This PR basically keeps the original string in the clipboard, rather than copying and pasting the formatted values.
Validation Steps Performed
- Tested it manually
@htcfreek I've made some changes to the files and opened a draft PR. Could you have a look at it since the code is not working? The result that I'm getting now is -
- Copied text -
something,that,i,know,about - Formatted text ("Paste as JSON") -
something,that,i,know,about
It doesn't do anything. Could you help me with it? Btw, issue is #33944
There is this SendPasteKeyCombination setting. If it is disabled your code can't work as you reset the clipboard before pasting.
@htcfreek
I think the issue is something else. Because even if the setting is false, the output still doesn't align. Let me explain -
- When we press the "Paste to JSON option", the function is triggered.
- The original value is copied into the
tempTextvariable. Also, since it's just agetfunction, I guess there is no modification in the clipboard data itself. - The same clipboard data is passed to the designated function and the result is stored in the
jsonTextvariable. - Now, we set the
jsonTextto the clipboard data. - Now, let's have the benefit of the doubt, and say the method inside if is not executed.
- We set the
tempTextto the clipboard data.
That doesn't explain why it pastes the original data. It shouldn't, right? Or that I think. I would love to hear from you regarding this.
Hi @htcfreek, I added some log statements in between to see what was stored in different locations. And, I have attached the resulting image of the logs.
internal void ToJsonFunction(bool pasteAlways = false)
{
try
{
Logger.LogTrace();
string tempText = ClipboardData.GetTextAsync().GetAwaiter().GetResult();
Logger.LogInfo("Temp text: " + tempText);
string jsonText = JsonHelper.ToJsonFromXmlOrCsv(ClipboardData);
Logger.LogInfo("JSON text: " + jsonText);
SetClipboardContentAndHideWindow(jsonText);
Logger.LogInfo("Copied JSON text to the clipboard");
if (pasteAlways || _userSettings.SendPasteKeyCombination)
{
Logger.LogInfo("Clipboard data: " + ClipboardData.GetTextAsync().GetAwaiter().GetResult());
Logger.LogInfo("Inside paste if");
ClipboardHelper.SendPasteKeyCombination();
Logger.LogInfo("End paste if");
}
SetClipboardContentAndHideWindow(tempText);
string temp = ClipboardData.GetTextAsync().GetAwaiter().GetResult();
Logger.LogInfo("Text in clipboard: " + temp);
}
catch
{
}
}
One question, that I was having, how come the clipboard content on paste changes to some other value or am I missing something?
Also, the last log statement has not been executed. I'm not sure about this behavior.
Also, why I used the .GetAwaiter().GetResult(): https://stackoverflow.com/a/44578884/12634085
@GhostVaibhav I was out of office for some time and currently I am a bit busy. Hopefully I have time to debug your changes next weekend.
@GhostVaibhav I am building a debug build now. Then I can investigate things tomorrow. (One first idea I have regarding your logging is, that the logger might not live long enough after closing the window.)
@GhostVaibhav After doing some investigation I can tell you the following:
- Your code works great (see logfile) except of a small timing problem: Executing the paste key needs to much time and clipboard gets reset before the paste action was executed.
- You need to add a
thread.Sleep()between sending the paste key and resetting the clipboard inOptionsViewModel::ToJsonFunction. I have succesfully tested it with 250 ms.
- You need to add a
- That you don't see the last log line "Text in clipboard:" with your modified code can have two reasons:
- A crash in
ClipboardHelper::SetClipboardTextContentthat is not catched in a try-catch block. - The clipboard is still accessed/locked by the set clipboard command and your code line
string temp = ClipboardData.GetTextAsync().GetAwaiter().GetResult();inToJsonFunctionmethod crashes.
- A crash in
- There is a big timing problem in
ClipboardHelper::SetClipboardTextContentwhich causesClipboard.Flush()to mostly not work.- Can be fixed by adding a while loop that waits until the clipboard update has finished.
Logs and investigation informations
Code changes are based on your public PR code.
Changes on OptionsViewModel::ToJsonFunction
- Added thread.Sleep command.
- Added two comments.
- Added log to know if the end of the method was reached.
ClipboardHelper.SendPasteKeyCombination();
}
// Sleeping 250 ms that paste command can be completely executed before resetting the clipboard.
// Otherwise we reset the clipboard to early and past the original content. Or the clipboard is still used while resetting and restting fails.
Thread.Sleep(250);
// Resetting clipboard content
SetClipboardContentAndHideWindow(tempText);
Logger.LogInfo("End of method 'ToJsonFunction' reached.");
Changes on ClipboardHelper::SetClipboardTextContent
- Added logging for the new content to set and the clipboard content after the set command. (For easier debugging.)
- Added logging if the set clipboard command returned true or false.
- Improved logging of errors for
Clipboard.Flush()by adding a try-catch inside the Task command. - Added a while loop to wait until the clipboard text is set completely.
internal static void SetClipboardTextContent(string text)
{
Logger.LogTrace();
if (!string.IsNullOrEmpty(text))
{
Logger.LogDebug("Text to set to clipboard: " + text);
DataPackage output = new();
output.SetText(text);
bool success = Clipboard.SetContentWithOptions(output, null);
Logger.LogInfo("Setting Clipboard data was success? : " + success);
// Wait 50 ms in a loop until setting the clipboard content has really finished.
while (!Clipboard.GetContent().Contains("Text"))
{
Thread.Sleep(50);
}
try
{
string clipContent = Clipboard.GetContent().GetTextAsync().GetAwaiter().GetResult();
Logger.LogInfo("Clipboard content for current process:" + clipContent);
}
catch (Exception ex)
{
Logger.LogError("Falied to get clipboard content: ", ex.GetBaseException());
}
// TODO(stefan): For some reason Flush() fails from time to time when directly activated via hotkey.
// Calling inside a loop makes it work.
bool flushed = false;
for (int i = 0; i < 5; i++)
{
if (flushed)
{
break;
}
try
{
Task.Run(() =>
{
try
{
Clipboard.Flush();
}
catch (Exception ex)
{
Logger.LogError("Clipboard.Flush() failed. Real reason:", ex.GetBaseException());
throw;
}
}).Wait();
flushed = true;
Logger.LogDebug("Clipboard flushed.");
}
catch (Exception ex)
{
Logger.LogError("Clipboard.Flush() failed", ex);
}
}
}
}
Log ouput
- Note: The originally copied text was
abcdef dsa dsaldsa.
[12:49:06.5536638] [Debug] JsonHelper::ToJsonFromXmlOrCsv
Convert from plain text.
[12:49:06.5537349] [Trace] ClipboardHelper::SetClipboardTextContent
[12:49:06.5537628] [Debug] ClipboardHelper::SetClipboardTextContent
Text to set to clipboard: [
"abcdef dsa dsaldsa"
]
[12:49:06.5542858] [Info] ClipboardHelper::SetClipboardTextContent
Setting Clipboard data was success? : True
[12:49:06.5838881] [Info] ClipboardHelper::SetClipboardTextContent
Clipboard content for current process:[
"abcdef dsa dsaldsa"
]
[12:49:06.6040774] [Debug] ClipboardHelper::SetClipboardTextContent
Clipboard flushed.
[12:49:06.7492502] [Trace] ClipboardHelper::SendPasteKeyCombination
[12:49:06.7908409] [Info] ClipboardHelper::SendPasteKeyCombination
Paste sent
[12:49:06.7912791] [Trace] ClipboardHelper::SetClipboardTextContent
[12:49:06.7913137] [Debug] ClipboardHelper::SetClipboardTextContent
Text to set to clipboard: abcdef dsa dsaldsa
[12:49:06.8134983] [Info] ClipboardHelper::SetClipboardTextContent
Setting Clipboard data was success? : True
[12:49:06.9234363] [Info] ClipboardHelper::SetClipboardTextContent
Clipboard content for current process:abcdef dsa dsaldsa
[12:49:06.9561999] [Debug] ClipboardHelper::SetClipboardTextContent
Clipboard flushed.
[12:49:06.9986701] [Info] OptionsViewModel::ToJsonFunction
End of method 'ToJsonFunction' reached.
@stefansjfw
- Should I open an new issue and PR to implement the loop inside all set clipboard methods to fix the flush problem?
- Do you have a better idea than using
Thread.Sleep()for waiting until the paste action is finally executed?
Hi @htcfreek, thanks for the detailed research and the huge write-up. As per it, I re-implemented both files per your suggestions, but they are still not working for me.
So now, when I copy the text and paste the JSON. The correct JSON is pasted, but the clipboard content is not updated. The clipboard contains the pasted JSON itself.
Also, I thought that this could be a timing issue, so I increased the thread's sleep time from 250 to 500, but it is still not working.
@GhostVaibhav Can you share the logging output?
The logging output is huge. Therefore, I have attached the whole log file. Fyi, it's filled with flush errors.
Also, when I'm pasting, the first time it's working fine. But after the second time, it's getting problematic.
Also, below is the output file for which the logs are attached.
this,is,something,that,i,know,off
[
[
"this",
"is",
"something",
"that",
"i",
"know",
"off"
]
]
[
[
"this",
"is",
"something",
"that",
"i",
"know",
"off"
]
]
[
"[",
" [",
" \"this\",",
" \"is\",",
" \"something\",",
" \"that\",",
" \"i\",",
" \"know\",",
" \"off\"",
" ]",
"]"
]
[
"[",
" [",
" \"this\",",
" \"is\",",
" \"something\",",
" \"that\",",
" \"i\",",
" \"know\",",
" \"off\"",
" ]",
"]"
]
[
"[",
" \"[\",",
" \" [\",",
" \" \\\"this\\\",\",",
" \" \\\"is\\\",\",",
" \" \\\"something\\\",\",",
" \" \\\"that\\\",\",",
" \" \\\"i\\\",\",",
" \" \\\"know\\\",\",",
" \" \\\"off\\\"\",",
" \" ]\",",
" \"]\"",
"]"
]
@GhostVaibhav Wow that's strange: The clipboard data gets updated. But when flushing the clipboard to make the content accessible by other apps it throws a strange error:
Clipboard.Flush() failed. Real reason:
Unspecified error
The operation is not permitted because the calling application is not the owner of the data on the clipboard.
Inner exception:
@GhostVaibhav After finding this article I am wondering if we can do the following two things:
- Instead of setting and then flushing using
Clipboard.SetDataObject(data, true). - After 1 validating the content with
bool isOriginalDataObject = Clipboard.IsCurrent(data);instead the way I suggested. Because comparing the whole object validates data and data type.
Hi @htcfreek, it now works. I made some changes to your code.
- The most important change I added was a sleep of 50 ms before doing another
Clipboard.Flush().
try
{
Task.Run(() =>
{
try
{
Clipboard.Flush();
}
catch (Exception ex)
{
Logger.LogError("Clipboard.Flush() failed. Real reason:", ex.GetBaseException());
throw;
}
}).Wait();
flushed = true;
Logger.LogDebug("Clipboard flushed.");
}
catch (Exception ex)
{
Logger.LogError("Clipboard.Flush() failed", ex);
Thread.Sleep(50); // This
}
- Also, you forgot to use the variable in the checking of the clipboard getting updated, so I changed that as well.
// Wait 50 ms in a loop until setting the clipboard content has finished.
while (!Clipboard.GetContent().GetTextAsync().GetAwaiter().GetResult().Contains(text))
{
Thread.Sleep(50);
}
But there are still exceptions about flush in the log file. Should that be of any concern now?
@GhostVaibhav If it now works I am fine with it. Do we need the adjustments for plain text and markdown too?
The changes are done in ClipboardHelper.cs which is then used in all three functions, so, I think, the answer is a yes.
@GhostVaibhav Can you commit and push please.
And aren't there different methods for html, text and image in clipboard helper?
@htcfreek I've pushed the code, you can have a look. Also, there are different methods for those things, but all the markdown, JSON, and text functions use the same text method.
@check-spelling-bot Report
:red_circle: Please review
See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.
Unrecognized words (2)
Falied restting
Some files were automatically ignored :see_no_evil:
These sample patterns would exclude them:
/modules/launcher/Plugins/Microsoft\.PowerToys\.Run\.Plugin\.TimeDate/Properties/[^/]+$
You should consider adding them to:
.github/actions/spell-check/excludes.txt
File matching is via Perl regular expressions.
To check these files, more of their words need to be in the dictionary than not. You can use patterns.txt to exclude portions, add items to the dictionary (e.g. by adding them to allow.txt), or fix typos.
To accept these unrecognized words as correct and update file exclusions, you could run the following commands
... in a clone of the [email protected]:GhostVaibhav/PowerToys.git repository
on the ghostvaibhav/33944 branch (:information_source: how do I use this?):
curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.22/apply.pl' |
perl - 'https://github.com/microsoft/PowerToys/actions/runs/10654954682/attempts/1'
Available :books: dictionaries could cover words (expected and unrecognized) not in the :blue_book: dictionary
This includes both expected items (1896) from .github/actions/spell-check/expect.txt and unrecognized words (2)
| Dictionary | Entries | Covers | Uniquely |
|---|---|---|---|
| cspell:r/src/r.txt | 543 | 1 | 1 |
| cspell:cpp/src/people.txt | 23 | 1 | |
| cspell:cpp/src/ecosystem.txt | 51 | 1 |
Consider adding them (in .github/workflows/spelling2.yml) for uses: check-spelling/[email protected] in its with:
with:
extra_dictionaries:
cspell:r/src/r.txt
cspell:cpp/src/people.txt
cspell:cpp/src/ecosystem.txt
To stop checking additional dictionaries, add (in .github/workflows/spelling2.yml) for uses: check-spelling/[email protected] in its with:
check_extra_dictionaries: ''
Warnings (2)
See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.
| :information_source: Warnings | Count |
|---|---|
| :information_source: binary-file | 2 |
| :information_source: non-alpha-in-dictionary | 1 |
See :information_source: Event descriptions for more information.
If the flagged items are :exploding_head: false positives
If items relate to a ...
-
binary file (or some other file you wouldn't want to check at all).
Please add a file path to the
excludes.txtfile matching the containing file.File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
^refers to the file's path from the root of the repository, so^README\.md$would exclude README.md (on whichever branch you're using). -
well-formed pattern.
If you can write a pattern that would match it, try adding it to the
patterns.txtfile.Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.
Note that patterns can't match multiline strings.
Before you merge plese check this similar PR: #36404
The root cause of this issue lies in continuously reading content from the clipboard and forcibly converting it into JSON. The correct and straightforward solution is to first check whether the content is already in the desired format, and only perform the conversion if it is not.
Closing this as we have another solution merged yet.