New color picker module - integrated from github.com/martinchrzan/Col… #4778
Conversation
# Conflicts: # PowerToys.sln
There was a problem hiding this comment.
Sorry for the delay on this.
This feature looks awesome! There are a couple nit things that I think we should be fix if we want to release it to .20.
- It would be great if you could set the the ColorPicker and ColorPickerUI Projects to a depenency of the runner to support an F5 and run scenario. eg.
- [New Bug] - The colorpicker.exe process doesn't quit when we terminate PowerToys.exe
- [New Bug] - The cursor gets permanently changed for some windows event after shutting down the process.
- [Opinion] - We may want to consider a different default Activation Shortcut as I find Ctrl interfers with copy paste functionality.
Note: I just kicked off an official build and would like to do a quick smoke test from the msi generated there, but I'm able to build and run from the installer locally.
I'm happy to approve this if we can commit to fixing the minor bugs I mentioned. @crutkas alternatively we could consider feature flags to be able to move forward without introducing bugs we don't have capacity for for .20.
This is awesome work thank you so much!!
…cker project into runner dependencies, restoring cursors on exit, added ManagedCommon as a dependency into installer
|
@ryanbodrug-microsoft I have addressed your comments:
Thanks a lot for the review, I know it is quite a lot of code to go through :) |
|
@martinchrzan Thanks for the updates, and sorry for the delay. Wrt #3 I'll try to find some repro steps for that, and document a new issue. It wasn't able to run from the installer, and I tracked it down to attempting to load the wrong architecture for MangedCommon in release. I've pushed those changes in a user branch at commit cf2ffca. I also created a PR #5046 that has a the above change change in master that fixed the broken installer that was preventing me from testing. I've used this to kick off an official build but that change is already in master. I'm just going to do 1 quick smoke test on the official build and then I'll sign of -- trusting you to merge in cf2ffca before completing the PR. I didn't want to push it directly to your branch without you approving it. |
* Changing configuration to x64 instead of AnyCPU. The previous configuration was preventing the ManagedCommon binary from being loaded in Release. * Updating MSI Installer with new icons (#4998)
|
@ryanbodrug-microsoft Thank you for the investigation! I wasn't aware of it. I merged your changes in and waiting for your approval :) |
There was a problem hiding this comment.
I validated the latest changes locally! Thanks for that. I also kicked off a new build. It would be good to double check that before you complete the PR, but I'm very confident it will work now based on the changes you just added. Thanks again for all the work on this :)
Summary of the Pull Request
NEW MODULE - Color Picker - integrated from github.com/martinchrzan/ColorPicker
Basic functionality provided:
Color picker appears when activation shortcut pressed (configurable in the settings)
Color picker follows the mouse cursor and shows the actual color that is below the cursor
Scroll up will cause Zoom Window to open for a better color picking precision
Left mouse click will copy that color into a clipboard in a predefined format (setting)
Changes cursor when picking a color (can be turned off)
Color picker is multimonitor/multi DPI aware. It respect monitors' boundaries and stays always in the view (predefined safe zones in top, bottom, left, right sides of a monitor)
Added the new section into Settings:
References
This is the first version of a color picker as discussed in
#864
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
Manual testing