Add Global Eyedropper for Windows #3801
Add Global Eyedropper for Windows #3801yush-1018 wants to merge 7 commits intoGraphiteEditor:masterfrom
Conversation
Summary of ChangesHello @yush-1018, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature for Windows users: a global eyedropper tool. This tool enhances the color selection capabilities by allowing users to pick colors from anywhere on their screen, not just within the application. It provides a visual magnifier for precision and integrates the sampled colors directly into the editor's color palette, streamlining the design workflow. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The addition of a global eyedropper for Windows is a great feature. However, the current implementation has several critical issues regarding performance and correctness. The most significant issue is the redundant creation and destruction of the magnifier window on every mouse move when the cursor is outside the editor viewport, which will cause severe performance degradation and resource churn. Additionally, the GDI drawing logic incorrectly uses color values as brush handles, which will prevent the magnifier from displaying the correct colors. There are also concerns about the magnifier sampling itself and the efficiency of using GetPixel in a loop.
| DesktopFrontendMessage::GlobalEyedropper { open, primary } => { | ||
| if open { | ||
| self.app_event_scheduler.schedule(AppEvent::StartGlobalEyedropper { primary }); | ||
| } else { | ||
| self.global_eyedropper.stop(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The global eyedropper window is being recreated on every PointerMove event when the cursor is outside the viewport because eyedropper_tool.rs spams this message. This causes significant performance overhead and resource churn. You should check if the eyedropper is already active before scheduling a start event.
DesktopFrontendMessage::GlobalEyedropper { open, primary } => {
if open {
if !self.global_eyedropper.is_active() {
self.app_event_scheduler.schedule(AppEvent::StartGlobalEyedropper { primary });
}
} else {
self.global_eyedropper.stop();
}
}
desktop/src/global_eyedropper.rs
Outdated
| right: ((x + 1) * pixel_size) as i32, | ||
| bottom: ((y + 1) * pixel_size) as i32, | ||
| }; | ||
| windows::Win32::Graphics::Gdi::FillRect(window_dc, &rect, windows::Win32::Graphics::Gdi::HBRUSH((color.0 + 1) as isize)); |
There was a problem hiding this comment.
Using HBRUSH((color.0 + 1) as isize) is incorrect for arbitrary colors. In GDI, this syntax only works for system color indices (e.g., COLOR_WINDOW + 1). For arbitrary RGB colors, you must create a solid brush using CreateSolidBrush and ensure it is deleted afterwards to avoid GDI object leaks.
let brush = windows::Win32::Graphics::Gdi::CreateSolidBrush(color);
windows::Win32::Graphics::Gdi::FillRect(window_dc, &rect, brush);
let _ = windows::Win32::Graphics::Gdi::DeleteObject(brush);
desktop/src/global_eyedropper.rs
Outdated
| for x in 0..res { | ||
| let sx = pt.x - (res as i32 / 2) + x as i32; | ||
| let sy = pt.y - (res as i32 / 2) + y as i32; | ||
| let color = GetPixel(desktop_dc, sx, sy); |
There was a problem hiding this comment.
Calling GetPixel 121 times per frame is very inefficient. Additionally, since the magnifier window is moved to the cursor position before rendering, GetPixel on the desktop DC will likely sample the magnifier window itself rather than the content beneath it. Consider capturing the screen area into a memory DC before moving/showing the magnifier, or using StretchBlt which is significantly faster.
desktop/src/global_eyedropper.rs
Outdated
| right: ((mid + 1) * pixel_size) as i32, | ||
| bottom: ((mid + 1) * pixel_size) as i32, | ||
| }; | ||
| windows::Win32::Graphics::Gdi::FrameRect(window_dc, &rect, windows::Win32::Graphics::Gdi::HBRUSH(1)); |
There was a problem hiding this comment.
HBRUSH(1) corresponds to COLOR_SCROLLBAR + 1. If you intended to draw a black or neutral frame, it's better to use a stock object like BLACK_BRUSH.
let brush = windows::Win32::Graphics::Gdi::GetStockObject(windows::Win32::Graphics::Gdi::BLACK_BRUSH);
windows::Win32::Graphics::Gdi::FrameRect(window_dc, &rect, windows::Win32::Graphics::Gdi::HBRUSH(brush.0));
desktop/src/global_eyedropper.rs
Outdated
| let g = ((pixel.0 >> 8) & 0xFF) as f32 / 255.0; | ||
| let b = ((pixel.0 >> 16) & 0xFF) as f32 / 255.0; | ||
|
|
||
| Some(Color::from_rgbaf32(r, g, b, 1.0).unwrap()) |
There was a problem hiding this comment.
Since the RGB values are derived from u8 division, they are guaranteed to be within the valid range [0, 1]. You can use from_rgbaf32_unchecked to avoid the overhead of checks and the unwrap() call.
Some(Color::from_rgbaf32_unchecked(r, g, b, 1.0))
|
Addressed all review feedback. Thanks! |
|
Looks like this doesn't compile. The global eyedropper feature will need to be designed in a way that allows for a consistent API for all platforms. Currently all platforms specific APIs are abstracted away with the native window handle. Ideally we would also do that for the eyedropper or make that more generic, something like a platform abstraction. Implementation for Wayland/mac needs some research (this PR can still only implement Windows support). |
yush-1018
force-pushed
the
feat/global-eyedropper
branch
from
e8103c5 to
7abc34a
Compare
…opper # Conflicts: # desktop/src/app.rs # desktop/wrapper/src/intercept_frontend_message.rs # desktop/wrapper/src/messages.rs
| } | ||
|
|
||
| fn render(&self) { | ||
| let Some(_window) = &self.window else { return }; |
There was a problem hiding this comment.
the window variable is unused, consider using this
if self.window.is_none() { return; }
| let window_hwnd = self.window_hwnd(); | ||
| let window_dc = GetDC(window_hwnd); | ||
|
|
||
| let mem_dc = CreateCompatibleDC(desktop_dc); |
There was a problem hiding this comment.
I see that you are doing this create and delete cycle on every render, Consider using the below code for optimization , basically instead do caching in GlobalEyedropperImpl struct . Since render happens very frequently , this should be a good caching optimization
pub(crate) struct GlobalEyedropperImpl {
window: Option<Window>,
primary: bool,
mem_dc: HDC, // Created once in start()
bitmap: HBITMAP, // Created once in start()
}
fn start(&mut self, ...) {
// Create window...
// Allocate GDI resources ONCE
let desktop_dc = GetDC(HWND::default());
self.mem_dc = CreateCompatibleDC(desktop_dc);
self.bitmap = CreateCompatibleBitmap(desktop_dc, 11, 11);
SelectObject(self.mem_dc, self.bitmap);
ReleaseDC(HWND::default(), desktop_dc);
}
fn render(&self) {
// Just use the cached resources - no allocation
StretchBlt(self.mem_dc, ...);
StretchBlt(window_dc, ...
}
fn stop(&mut self) {
// Deallocate ONCE when eyedropper closes
DeleteObject(self.bitmap);
DeleteDC(self.mem_dc);
self.window = None;
}
There was a problem hiding this comment.
can this and the linux.rs be absracted ? Not super sure on this but the code is duplicated here
|
What Timon mentioned is correct and I am not seeing how this PR is likely to have a path towards being merged because its approach does not follow one that meets our required direction. I think the best option is to close it and discuss any plans towards a proper implementation, if interest arises, in an issue rather than this PR. Thanks for the attempt. |
4 participants
Added a global eyedropper tool for the desktop app on Windows.
Changes include: