Add Global Eyedropper for Windows #3801
Add Global Eyedropper for Windows #3801yush-1018 wants to merge 1 commit 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();
}
}| 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);| 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.
| 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));| 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))1 participant
Added a global eyedropper tool for the desktop app on Windows.
Changes include: