microsoft
[PTRun] Improve "Copy failed" message boxes
Description of the new feature / enhancement
Currently the MsgBox just says Copy failed, I think it would be beneficial if it also provided the user with the error message. Applies to value generator, calculator, unit converter plugins
Scenario when this would be used?
Trying to find out why I can't copy something.
Supporting information
No response
@PesBandi Your contribution is welcome and thank you for your work on this. Feel free to open a PR. If you have any further questions don't worry to ask them.
Maybe applies to "Time and Date" plugin too. We should align all built-in plugin in how they copy things and how they handle errors.
Maybe applies to "Time and Date" plugin too. We should align all built-in plugin in how they copy things and how they handle errors.
I didn't mention time and date, because it currently doesn't MessageBox, it just logs the exception. Should I change that?
https://github.com/microsoft/PowerToys/blob/d51ca9f8d4ebf15dee7060315e75aca3ccf941f1/src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.TimeDate/Components/ResultHelper.cs#L48-#L62
Maybe applies to "Time and Date" plugin too. We should align all built-in plugin in how they copy things and how they handle errors.
I didn't mention time and date, because it currently doesn't MessageBox, it just logs the exception. Should I change that?
https://github.com/microsoft/PowerToys/blob/d51ca9f8d4ebf15dee7060315e75aca3ccf941f1/src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.TimeDate/Components/ResultHelper.cs#L48-#L62
Don't think we need to change the current behavior
Hi @htcfreek, thanks for the response. At first I also didn't want to change it, but now I've realized that there are a couple of issues with the current approach:
- As the comment says, the code was copied from the TimeZone plugin and therefore isn't consistent with the rest of the plugins.
- If copying fails, it doesn't tell the user in any way.
- Not sure about this one, but is the
Clipboard.Clear()call really necessary?
IMO it would be better to make it consistent with the rest, let me know what you think.
@PesBandi Aligning the code makes sense and I think it is a benefit for the users to see an error message if copying fails. So please update the "Date and Time" plugin.
Regarding the Clipboard.Clear() command: According to the docs we don't neeed it as the set command clears the clipboard anyway. But for strangeness we never had issue reports about copy errors for the "Date and Time" plugin. So I wondering if clearing before setting is the solution for our problems. 🤔
And please align the code over all plugins that we log copy errors including the exception if we don't do that everywhere. That may help with further investigation.
But for strangeness we never had issue reports about copy errors for the "Date and Time" plugin. So I wondering if clearing before setting is the solution for our problems. 🤔
Yeah, you're right. Almost all of the bug reports are about the Calculator plugin. One thing to consider is that there aren't any reports about Value Generator and very few for Unit Converter. That's why I think that it's a Calculator specific problem and doesn't have anything to do with clearing the clipboard. These are just speculations though, I can't find what the actual problem was, since the errors aren't logged.
One more thing to mention: when I did some testing, using Clear() actually worsened the number of successful copy operations somehow? (I was getting CLIPBRD_E_CANT_OPEN)
At this point I think the best approach would be to add the logging to all plugins and wait for what the users report.
At this point I think the best approach would be to add the logging to all plugins and wait for what the users report.
@PesBandi, I do report CLIPBRD_E_CANT_OPEN as well :-) It's not Calculator specific, it rather depends on how exactly the copying is done with a combination of some machine-specific configuration. See #25437 and https://github.com/dotnet/wpf/issues/9901
Hi @josefblaha, thanks for the references. I have actually read #25437 while working on this issue, but I didn't know what to do with any of that information. All the relevant info from that thread is summed up in your comment, however it is different from what I observed. As I already mentioned, calling Clipboard.Clear() before SetText() does absolutely nothing on my machine, and from what I read in https://github.com/dotnet/wpf/issues/9901 the clipboard behavior just randomly changes. That's why ignored it, after all I was improving just the message boxes, not the copying itself 😁
Anyway, what do you recommend we do now? Keep it the way it is now? Do we add a Clear() call everywhere? Or do we use SetDataObject()? I really don't know...
Also, why do you think such an overwhelming majority of the bug reports are about Calculator? Is it just used that much more compared to other plugins?