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

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

GhostVaibhav
Copy link
Contributor

Summary of the Pull Request

  • This PR fixes the overwriting of the formatted text again and again.

PR Checklist

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
Copy link
Contributor Author

GhostVaibhav commented Aug 4, 2024

@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

@htcfreek
Copy link
Collaborator

htcfreek commented Aug 4, 2024

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

@GhostVaibhav
Copy link
Contributor Author

@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
Copy link
Contributor Author

GhostVaibhav commented Aug 13, 2024

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

@htcfreek
Copy link
Collaborator

@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

This comment was marked as outdated.

@htcfreek
Copy link
Collaborator

htcfreek commented Aug 31, 2024

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

@GhostVaibhav
Copy link
Contributor Author

GhostVaibhav commented Sep 1, 2024

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.

@htcfreek
Copy link
Collaborator

htcfreek commented Sep 1, 2024

@GhostVaibhav
Can you share the logging output?

@GhostVaibhav
Copy link
Contributor Author

GhostVaibhav commented Sep 1, 2024

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\\\"\",",
  "  \"  ]\",",
  "  \"]\"",
  "]"
]

@htcfreek
Copy link
Collaborator

htcfreek commented Sep 1, 2024

@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
Copy link
Collaborator

htcfreek commented Sep 1, 2024

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

@GhostVaibhav
Copy link
Contributor Author

GhostVaibhav commented Sep 1, 2024

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?

@htcfreek
Copy link
Collaborator

htcfreek commented Sep 1, 2024

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

@GhostVaibhav
Copy link
Contributor Author

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

@htcfreek
Copy link
Collaborator

htcfreek commented Sep 1, 2024

@GhostVaibhav
Can you commit and push please.

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

@GhostVaibhav
Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Collaborator

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

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

Left some comments. Didn't tested again yet.

@GhostVaibhav GhostVaibhav marked this pull request as ready for review September 1, 2024 16:20
@htcfreek htcfreek added the Needs-Review This Pull Request awaits the review of a maintainer. label Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants