-
Notifications
You must be signed in to change notification settings - Fork 272
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.
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
Add colorsliders, UI and more... #961
Conversation
Added colorslider widgets + HSV, RGB colorsliders with extendability + Color wheel and color sliders are now separated + Improved performance of painting color square. + Remember color values from last session + Can modify existing swatches on bitmap (forced behaviour for now) + Ability to add colors to palette using the color wheel, and via the dialog through a new button. + Added colorpalette preference icon
@candyface Man great work! looks really nice I was wondering while you were tinkering with the palette, did you come across the code for this issue? #942 I think scribblemaniac said the code for the remove button was already there but it's just that the button disappeared from the palette, and it's really annoying to have to delete the colors via xml and reimport the palette 😝 |
@Jose-Moreno I considered adding the remove button again but it makes the application unstable because it doesn't work properly for vector yet. Currently If you remove a swatch from the palette and switch to vector, the application will crash. The problem as I see it is that everything is connected to the palette, so before I can bring back that feature, I'll have to fix it for vector. |
Having looked through this #755 issue, I'm quite certain it's been fixed in this PR too. Modifying colors and switching between vector and bitmap will not affect each other anymore. |
Awesome @candyface! Will review all these PRs this weekend, great job! |
Reviewing |
@@ -65,6 +66,25 @@ void ColorPaletteWidget::initUI() | |||
else | |||
setGridMode(); | |||
|
|||
QColor savedColor; | |||
savedColor.setRgba(settings.value("colorOfSliders").toUInt()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@candyface Could I ask what "colorOfSliders" is for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use "colorOfSliders as a QColor representative, to load the color settings from our previous session. The value is added to QSettings in colorbox initUI, and restored in ColorPaletteWidget initUI.
Do we already have a QSetting to save our QColor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not in QSettings. But there is ColorManager::frontColor()
, which is the active color for drawing. And it will be written to the xml when saving a project. So I'm wondering if we can just use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, yeah we should probably do that instead. it looks like we load the value from object in ColorManager, so we just have to get that value on init and then we should be fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I think it will be more consistent, not having 2 different color values in the system.
Also the front color may change when users load a new project, so the color inspector will need to get the new color when Editor::objectLoaded()
signal is emitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright I'll change it later today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added and tested that it works now.
I'm not sure the implementation is the best way to go though. I've added a new connection that is called for the only purpose of loading our color on init. This avoids setColor changing our value back and fourth between values, as it may at some point get a value which it can't parse correctly.
It's the least messy way I can come up with for now and it avoids adding too much logic to setColor.
…itting - ex. Spinboxes may emit valueChanged when setting a new range, end up sending a invalid color value
Color and ui
Well done @candyface |
dialog through a new button.