PowerToys icon indicating copy to clipboard operation
PowerToys copied to clipboard

[Advanced Paste] Repeatedly pasting plain text as JSON produces inconsistent results

Open GhostVaibhav opened this issue 1 year ago • 20 comments

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] 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

GhostVaibhav avatar Aug 04 '24 17:08 GhostVaibhav

@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

GhostVaibhav avatar Aug 04 '24 17:08 GhostVaibhav

There is this SendPasteKeyCombination setting. If it is disabled your code can't work as you reset the clipboard before pasting.

htcfreek avatar Aug 04 '24 21:08 htcfreek

@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 tempText variable. Also, since it's just a get function, 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 jsonText variable.
  • Now, we set the jsonText to the clipboard data.
  • Now, let's have the benefit of the doubt, and say the method inside if is not executed.
  • We set the tempText to 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.

GhostVaibhav avatar Aug 05 '24 16:08 GhostVaibhav

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
    {
    }
}

image

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 avatar Aug 13 '24 16:08 GhostVaibhav

@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.

htcfreek avatar Aug 24 '24 15:08 htcfreek

@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.)

htcfreek avatar Aug 30 '24 20:08 htcfreek

@GhostVaibhav After doing some investigation I can tell you the following:

  1. 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 in OptionsViewModel::ToJsonFunction. I have succesfully tested it with 250 ms.
  2. That you don't see the last log line "Text in clipboard:" with your modified code can have two reasons:
    1. A crash in ClipboardHelper::SetClipboardTextContent that is not catched in a try-catch block.
    2. The clipboard is still accessed/locked by the set clipboard command and your code line string temp = ClipboardData.GetTextAsync().GetAwaiter().GetResult(); in ToJsonFunction method crashes.
  3. There is a big timing problem in ClipboardHelper::SetClipboardTextContent which causes Clipboard.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

  1. Should I open an new issue and PR to implement the loop inside all set clipboard methods to fix the flush problem?
  2. Do you have a better idea than using Thread.Sleep() for waiting until the paste action is finally executed?

htcfreek avatar Aug 31 '24 12:08 htcfreek

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 avatar Sep 01 '24 11:09 GhostVaibhav

@GhostVaibhav Can you share the logging output?

htcfreek avatar Sep 01 '24 11:09 htcfreek

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.

Log_2024-09-01.txt

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 avatar Sep 01 '24 11:09 GhostVaibhav

@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: 

htcfreek avatar Sep 01 '24 11:09 htcfreek

@GhostVaibhav After finding this article I am wondering if we can do the following two things:

  1. Instead of setting and then flushing using Clipboard.SetDataObject(data, true).
  2. 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.

htcfreek avatar Sep 01 '24 11:09 htcfreek

Hi @htcfreek, it now works. I made some changes to your code.

  1. 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
}
  1. 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 avatar Sep 01 '24 12:09 GhostVaibhav

@GhostVaibhav If it now works I am fine with it. Do we need the adjustments for plain text and markdown too?

htcfreek avatar Sep 01 '24 12:09 htcfreek

The changes are done in ClipboardHelper.cs which is then used in all three functions, so, I think, the answer is a yes.

GhostVaibhav avatar Sep 01 '24 13:09 GhostVaibhav

@GhostVaibhav Can you commit and push please.

And aren't there different methods for html, text and image in clipboard helper?

htcfreek avatar Sep 01 '24 14:09 htcfreek

@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.

GhostVaibhav avatar Sep 01 '24 14:09 GhostVaibhav

@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.txt file 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.txt file.

    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.

github-actions[bot] avatar Sep 01 '24 14:09 github-actions[bot]

Before you merge plese check this similar PR: #36404

htcfreek avatar Dec 18 '24 05:12 htcfreek

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.

shuaiyuanxx avatar Dec 18 '24 06:12 shuaiyuanxx

Closing this as we have another solution merged yet.

htcfreek avatar Dec 19 '24 11:12 htcfreek