Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Customizable ColorPicker #2202

Merged
merged 6 commits into from
Jan 30, 2022
Merged

Customizable ColorPicker #2202

merged 6 commits into from
Jan 30, 2022

Conversation

deo002
Copy link
Contributor

@deo002 deo002 commented Dec 29, 2021

Closes #1942

Functionality added:

  • Select preset from colorpicker using spinbox.
  • Press Delete to remove that preset from colorpicker.
  • Choose color from color-wheel or enter it manually.
  • Press Add to add that preset to colorpicker.

@borgmanJeremy
Copy link
Contributor

Thanks, this looks really good for a first pass. Since it touches so many place in the code and we are very close to the version 11 release I will wait to merge this until after the release.

Some comments based on testing (haven't done code review yet).

  • If I add too many colors I get an error that says "Unable to add preset". This should probably state that the error stems from a cap on how many colors can be added.
  • I think we should increase the number of colors to match the number that was added in NC: Add option for using large color palette on right button click #2171. @ThePurple I think then we could remove the build flag for the pallet size. How does that sound?
  • It would be nice if there were arrows or something that let you re-order the colors. This could be added in a future PR, not a blocking issue.

@deo002
Copy link
Contributor Author

deo002 commented Dec 31, 2021

Thanks @borgmanJeremy! Third one would be a really nice addition.

@panpuchkov
Copy link
Contributor

@borgmanJeremy it sounds great, thx.

@borgmanJeremy
Copy link
Contributor

Okay the version 11 issues are slowing down so I am ready to look at this / merge. Can you remind me where it left off? We just need to do a code review and some testing?

@deo002
Copy link
Contributor Author

deo002 commented Jan 27, 2022

@borgmanJeremy Yep, testing and code review are left.
Some changes were suggested too. I will implement them along with the changes you would suggest in the code review.
Thanks!

borgmanJeremy
borgmanJeremy suggested changes Jan 28, 2022
View reviewed changes
Copy link
Contributor

@borgmanJeremy borgmanJeremy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you this is a great feature!

I had some very minor comments. I had to fix some merge conflicts to do my testing but it was pretty easy.

I'll get this merged quickly once the comments are addressed.

src/widgets/colorpickerwidget.cpp Outdated
@@ -0,0 +1,139 @@
// SPDX-License-Identifier: GPL-3.0-or-later
// SPDX-FileCopyrightText: 2017-2019 Alejandro Sirgo Rica & Contributors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI for any new files that you create, you can list your self as the copyright owner since you authored it.

src/widgets/colorpickerwidget.cpp Outdated
c.setAlpha(100);
painter.setPen(c);
QRect highlight = m_colorAreaList.at(i);
highlight.moveTo(highlight.x() - 3, highlight.y() - 3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this "-3"? Maybe 3 as a const variable with a descriptive name so its easier to read in the future?

src/widgets/colorpickerwidget.cpp Outdated
QRect highlight = m_colorAreaList.at(i);
highlight.moveTo(highlight.x() - 3, highlight.y() - 3);
highlight.setHeight(highlight.height() + 6);
highlight.setWidth(highlight.width() + 6);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question on 6

src/widgets/spinbox.cpp Outdated
#include "src/widgets/spinbox.h"
#include "src/utils/confighandler.h"

SpinBox::SpinBox(QWidget* parent)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure SpinBox is the best name for this. Maybe put something in the name about what makes it different from a normal spin box? Like ColorSpinBox or something

src/config/colorpickereditor.cpp Outdated

colors << m_color;

const int maxPresetsAllowed = 10;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this maximum match #2171 so we can remove this compile time option.

src/config/colorpickereditor.cpp Outdated
const int maxPresetsAllowed = 10;

if (colors.size() > maxPresetsAllowed) {
QMessageBox::critical(this, tr("Error"), tr("Unable to add preset."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjust this error to state why the preset cannot be added "IE exceeded maximum" or something.

src/config/colorpickereditor.cpp Outdated

if (colors.size() < minPresetsAllowed) {
QMessageBox::critical(
this, tr("Error"), tr("Unable to remove preset."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjust this to say why it cannot be removed.

@deo002
Copy link
Contributor Author

deo002 commented Jan 30, 2022 •
edited
Loading

Thanks for feedback! I have pushed a fix addressing all the requested changes. @borgmanJeremy

@borgmanJeremy borgmanJeremy merged commit 259e438 into flameshot-org:master Jan 30, 2022
@borgmanJeremy
Copy link
Contributor

Thanks!

@borgmanJeremy borgmanJeremy mentioned this pull request Jan 30, 2022
Open
panpuchkov pushed a commit to namecheap/flameshot that referenced this pull request May 13, 2022
@deo002
Customizable ColorPicker (flameshot-org#2202)
9c9730f 
* Added spinbox, refactored colorpicker

* Added add preset functionality

* Added delete preset

* Refactored code

* Fix

(cherry picked from commit 259e438)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borgmanJeremy borgmanJeremy borgmanJeremy requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add black and white as default-color
3 participants
@deo002 @borgmanJeremy @panpuchkov