Project Ironsides - Move clipboard monitor to be a standalone tool #3512
Conversation
timkur
changed the title
timkur
changed the title
| { | ||
| var process = await InvokeToolInternal(targetProcessId, hWnd); | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
NIT: Catch InvalidOperationException instead because the exception InvokeToolInternal throws is InvalidOperationException
There was a problem hiding this comment.
Well, I don't want any exception to be unhandled here. Is there an advantage to only catching one type of exception?
There was a problem hiding this comment.
Readability and adding intent to the code. Catching InvalidOperationException tells readers that the method knows how to deal with that particular exception.
If the method can't do anything helpful for other exceptions, it should bubble up to a try/catch in the stack.
Also, InvokeToolExternal already catches Exception so InvokeTool will never get an Exception from InvokeToolsExternal.
There was a problem hiding this comment.
The code does pop an error message letting the user know that it couldn't launch a tool. Nobody further up the stack has anything more to say about it. For errors launching a tool, the buck stops here.
|
|
||
| public ClipboardMonitorControlViewModel() | ||
| { | ||
| _dispatcher = Microsoft.UI.Dispatching.DispatcherQueue.GetForCurrentThread(); |
There was a problem hiding this comment.
In the rest of Dev Home it's not good to use this, since we may not be on the UI thread, and therefore this may return null. Is different in PI?
There was a problem hiding this comment.
Yeah, in PI we always create the view model in the view's constructor. When does DevHome create its view models?
There was a problem hiding this comment.
Sometimes in view's constructor, but often in services registered with the host.
There was a problem hiding this comment.
Ahh... so your services push data to a singleton view models vs having the view models pull data from the services?
We did go down that path a bit, but since our views can come and go, it became a pain to "retarget" the view model to new views when they went away. So now our services/models maintain state, and view models can come and go and query that/register callbacks/etc when needed.
5 participants
Summary of the pull request
This moves the Clipboard monitoring tool out from an expando in the Extended Control and is treated like any other external tool. On launching it, a new undocked window opens that contains the clipboard monitor.
The clipboard monitor can be pinned/unpinned from the bar. However, it currently can't be unregistered like other tools.
Note, the goal here is to reduce folks dependencies on http://error and make it really quick and easy to do decimal<->hex conversions and lookup Windows error codes.
References and relevant issues
Detailed description of the pull request / Additional comments
While I have a command class for both external and internal tools (Tool.cs) I do have them separate for now. The biggest reason that I didn't try to combine the implementation even more is (a) I wanted to keep the clipboard monitor running in-proc, and (b) I didn't want settings persisted in the externaltools.json file that could be deleted/modified/etc, The code is pretty simple right now to take everything in externaltools.json wholesale, and didn't want to muck with that. So persistent settings, like if the Clipboard Monitor is pinned, is stored in app settings vs the JSON database.
This is only hooked up in the horizontal bar view. Rather than write duplicate code to put it in the vertical bar, I'm going to wait and hopefully will get it for free when the vertical bar moves to using a CommandBar.
Validation steps performed
PR checklist