flameshot-org
Add option to add 'eyedropper' color grabber to the color picker
This adds a checkbox to have the custom "picker" option in the colour picker immediately open and use the colour grabber instead of the side panel.
I find myself quite often wanting to use on-screen colours when editing in flameshot, and using the side panel for it is a bit more cumbersome than necessary, this option alleviates that by allowing use of the grabber directly, while the side panel remains available through usual means.
@Forkelt
Thanks for the PR. We usually prefer to have the features first discussed before having the PR. But thanks for the code. It makes sense to me and I think this is a good improvement. Let's get other devs' opinions and reviews.
@panpuchkov @FelixJochems @veracioux @jack9603301 what do you folks think about this and if you are positive about the change, can you also review the code.
Generally I also find myself quite often in the situation where I'd like to grab the color quickly, but in the same time using colors from the palette :smile:
Maybe you could modify the UX a little bit and add a picker button somewhere in the palette (in the middle maybe?). That could be enabled for everyone by default as it wouldn't break any previous usage patterns (so no new config option needed).
What do you think?
I had some other ideas about how this might be implemented, such as by adding a new option to the palette like @b0ch3nski mentioned, but from what I saw there's quite a bit that would have to be changed in how the picker works to enable that.
In general, I think opening the side panel from the picker makes very little sense, and the only reason I think for it doing that is because that's where the palette sits, but the side panel is a bit overkill for just that purpose. I think it might make more sense for the picker to open the palette by itself on screen, with perhaps the grabber button still present? But again, this would be a much more significant change (and would definitely require some consensus on whether that's a desirable functionality).
@mmahmoudian if the maintainers think this might be worth further discussion to find a better solution which might not have to add extra config options either, I'd be happy to engage in that. Apologies for the "cold PR", I believed it to be a relatively uncontroversial change and it wasn't a great deal of work.
Thanks @Forkelt for the explanation. I'm totally fine with both your current change in this PR and your other idea, although I'm not a maintainer :slightly_smiling_face:
IMO it makes great sense to get the color picker closer to the color palette itself.
@borgmanJeremy @jack9603301 @FelixJochems what is your opinion about this
I agree with the sentiment of the change, but I think it would be better to add another circle to the color picker with the eye drop SVG.
I agree @borgmanJeremy, as I said I decided to go for a smaller change. The current structure of the code for the colour picker especially makes that approach a lot more involved (and potentially more opinionated), but when I have some time to spend on this I'll have a look at it.
Hi @borgmanJeremy, @mmahmoudian. I've finally found enough boredom to get back to this ;) The new implementation adds the extra option with the eyedrop SVG as suggested, which can now be toggled from the colorpicker editor rather than in the regular config 'jungle'. The behaviour is otherwise unchanged.
Worth noting that there's an implicit setting that the regular 'picker' always exists, and the grabber and picker will always occupy indices 1 and 0 respectively. This is no different from the current state on master, where the 0 index was already reserved (as can be seen from some loops hard-coded to start from 1 rather than 0). This isn't necessarily wrong in my eyes, but it does conflict with the implication given by the config example that the picker can be disabled or re-ordered. Perhaps the picker and grabber should not be in that part of the config to make their special role explicit.
I think that's out of the scope of this PR though, but wanted to note it because this comes quite close to that behaviour.
I also considered the implementation of having the 'color picker' option just open a nice in-line colour wheel rather than the side-menu, which requires more mouse (and eye) movement, and I've experienced to be somewhat buggy on Windows with multiple displays, but this would be a big enough change to justify its own PR (or even RFC) and approval process.
Apologies for the extra change, I realised that I'd not fully implemented the 'grabber' option in the colour palette, and also that it was unnecessary since this already relies on the semantics of a 'non-colour' option being in the palette twice; saying the 'picker' option can be added a second time to enable the grabber makes these semantics more obvious.