Replace default gtk color picker #1025
Conversation
|
Good |
|
Decided why not, so I tried making a "live" color picker with a smaller version. Turning out well so far, I probably should have tried to give this a crack before opening PR. Can't figure out opacity, but it might just be nvidia+wayland messery. |
|
I think this now covers 761, because the color picker can now be left out as a separate window while editing.
Changing palette swatch still invokes blocking color picker. |
There was a problem hiding this comment.
This is really cool, thanks for working on this!
There's a lot of code to go through, so I've started by going through the changes in ColorExtensions
For the general approach, I think it would be best to:
- unify
Hsvwith the existingHsvColorstruct, which is hardly used anywhere so can be adjusted as needed - add new constructors / methods into
Color(and probably also removeRgbColor). This can become a more useful type in Pinta.Core, like we did in the past for Rectangle, Point, etc
|
Alright, I think i managed to get all the feedback implemented. Thanks for the feedback. In the meantime, I was playing around more with the opacity and I managed to get it to work! Turns out it wasn't wayland+nvidia, it just wasn't signaling to wayland (i think) that the window could be transparent, but setting opacity at 99.5% on dialog startup seems to resolve it :)
|
There was a problem hiding this comment.
Thanks! The color changes look good overall - just left a few followup comments. I'll try to look through the remaining code soon
I assume the huesaturation2.png test was just changing due to some minor numerical differences with moving HsvColor to use double?
| const int SWATCH_SIZE = 19; | ||
| const int WIDGET_HEIGHT = 42; | ||
| const int PALETTE_MARGIN = 10; | ||
| public const int PALETTE_ROWS = 2; |
There was a problem hiding this comment.
Rather than making all of these public (along with methods like GetSwatchAtLocation()), it might be cleaner to have a reusable palette widget that can be used both for the status bar and the dialog? Right now things seem tightly coupled / duplicated, with the color picker dialog having to carefully set up itself in the exact same manner as this widget
There was a problem hiding this comment.
Thanks for the updates!
I've gone through the dialog itself and left a few more comments. I think refactoring into a shared palette widget between this dialog and the status bar is something we could leave to another PR, since this one's quite large already :)
If you can address the remaining comments and rebase on the latest master branch, I think this should be ok to merge!
| /// <param name="mousePos">Position of the mouse relative to the top widget, usually obtained from Gtk.GestureClick</param> | ||
| /// <param name="relPos">Position of the mouse relative to the drawing area.</param> | ||
| /// <returns>Whether or not mouse position is within the drawing area.</returns> | ||
| public static bool IsMouseInDrawingArea (this Widget widget, Widget topWidget, PointD mousePos, out PointD relPos) |
There was a problem hiding this comment.
When you rebase on the master branch, this could go into Pinta.Core/Extensions/Gtk/GtkExtensions.Widget.cs which now contains all the extension methods for GTK widgets
| // uses a label, scale, and entry | ||
| // then hides the scale and draws over it | ||
| // with a drawingarea | ||
| public class ColorPickerSlider : Gtk.Box |
There was a problem hiding this comment.
There are quite a few compiler warnings in this file about nullability issues
| { | ||
| private readonly Gtk.Window top_window; | ||
|
|
||
| public Gtk.Label label = new Gtk.Label (); |
There was a problem hiding this comment.
Many of these fields look like they could be made private? If they are public, please use properties for them instead with UpperCase naming.
| } | ||
|
|
||
|
|
||
| private bool entryBeingEdited = false; |
There was a problem hiding this comment.
This member is unused - was this an alternate approach for the suppressEvent hack? Personally I'd prefer a flag that is turned on and off to suppress event handling, rather than relying on a magic number
|
|
||
| public class CheckboxOption : Gtk.Box | ||
| { | ||
| public bool state = false; |
There was a problem hiding this comment.
Similarly, please make these into properties if they need to be public. For the label, I don't think a reference to it needs to be kept as a member?
| private Gtk.Box color_display_box; | ||
|
|
||
| // palette | ||
| private int palette_display_size = 50; |
There was a problem hiding this comment.
Some of these can be made const
| } | ||
| } | ||
|
|
||
| var img = MemoryTexture.New (draw_width, draw_height, MemoryFormat.R8g8b8a8, Bytes.New (data), (UIntPtr) stride).ToSurface (); |
There was a problem hiding this comment.
There are a couple extra copies of the buffer here, so it would probably be more straightforward to create an ImageSurface of the right dimensions, and then use GetPixelData() to write directly to its pixels
| var colorDisplayList = new Gtk.ListBox (); | ||
|
|
||
| if (colors.Length > 1) { | ||
| var colorDisplaySwap = new Gtk.Button (); |
There was a problem hiding this comment.
Could you also add a tooltip for this button? I found it a bit hard to figure out from the icon what it did.
We could also add a better icon like this: https://fonts.google.com/icons?selected=Material+Symbols+Outlined:swap_vert:FILL@0;wght@400;GRAD@0;opsz@24&icon.query=swap&icon.size=24&icon.color=%23e8eaed which is from one of the icon sets we already use
I'm not sure if this fully covers #761 because it does not act like an overlay like PDN's color picker does, but it at least provides a superior color picker alternative.
This replaces the default gtk color picker with a custom one. It allows editing both primary and secondary, HSV color circle and Sat / Val square, RGB and HSV color sliders with color previewing and manual input entries.
I may try slimming it down and getting it as a non-blocking dialog so you dont have to keep reopening it and can keep it open a la PDN, but that would conflict with the recent palettes (Currently the recent palettes only update on OK, but if i change it to overlay, when should the recent palettes update? on any color change? i reckon that'd update way too fast.)