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

Fix remove color button and add context menu #971

Merged
merged 4 commits into from Jun 12, 2018

Conversation

Projects
None yet
2 participants
@CandyFace
Member

CandyFace commented May 19, 2018

it s back

I've fixed the remove button and added some enhancements.

  • Add, replace or remove using right-click through the context-menu shown above
  • Drag to select multiple color swatches
  • no auto color update on palette anymore, use replace via context menu.

When palette is empty, vector layer will use front color instead of palette.

Known bugs:
modifying first index in palette, doesn't update vector stroke for some reason..
this is an old bug but I wasn't able to find the cause this time around.
Considering my proposal #962 that changes this, I'm leaving the bug as is for now.

@chchwy

This comment has been minimized.

Member

chchwy commented May 20, 2018

One more thing needs to find out: What should pencil2d do when users remove a color which is in use?

  1. Open Pencil2D, go to a vector frame
  2. Pick color Red from color palette
  3. Use brush tool and draw something with Red color
  4. Remove Red color from color palette

In step 4, the color Red which the brush stroke connects to no longer exists, so the brush stroke color should change, but it doesn't.

Go to another frame and then go back to the previous drawn frame, the red brush stroke becomes Dark Red, the next color in the palette.

The behaviours are apparently not wanted.

A simple solution is telling users the color is in use, you can't remove it. This is what pencil2d 0.4.4 did. Or telling user they can bind the strokes to another palette color, not just silently bind to the next color.

@CandyFace

This comment has been minimized.

Member

CandyFace commented May 20, 2018

Non of those solutions are very good though, the user should be able to delete all colors in their palette and the vector stroke should not depend on that. To fix this properly though would require us to keep track of strokes and color that does not have a palette index anymore.

Considering the state of vector still, I ignored the problem but it's true that the user should have feedback as to what should happen, since the stroke is bound to a palette color.

I will add the behaviours you speak of for now, so if the color is in use, the user will see a popup where they can choose to bind to next color in palette otherwise they can't remove it.

@chchwy

This comment has been minimized.

Member

chchwy commented May 20, 2018

Binding vector stroke's color to the color palette is a feature though, I am not sure why you think the strokes shouldn't depend to color palette? It gives users the freedom to determine and change color later.

But I agree with you that users should be able to delete some palette colors. In this case, we have to provide a way that can re-bind the strokes to another color of the palette.

@CandyFace

This comment has been minimized.

Member

CandyFace commented May 20, 2018

I don't think the strokes should depend on the palette because it conflicts with the standard palette behaviour as seen in any other drawing application. There's no way of telling which or if a stroke is bound to a color, except the color itself.

Binding vector stroke's color to the color palette is a feature though, I am not sure why you think the strokes shouldn't depend to color palette? It gives users the freedom to determine and change color later.

If the user decides to change a swatch color on bitmap for example, then the vector color will change too or lose its connection to palette, which is restrictive. The imo. more user friendly solution would be to keep the color palette static and keep track of vector strokes somewhere else.

Alternatively we could have a palette for both bitmap and vector.

The problem that we're currently facing:
The user will expect a similar behaviour no matter if they use vector or bitmap because there's nothing to suggest the contrary. so we could make it explicit that the color palette has different behaviours depending on the layer you use but still share the same palette, or we could simply separate the behaviours, this is what I'm suggesting with #962

we have to provide a way that can re-bind the strokes to another color of the palette.

Agreed.

@chchwy

This comment has been minimized.

Member

chchwy commented May 20, 2018

Mmm...It's a feature in Toom boom, take a look at this video:
https://www.toonboom.com/resources/video-tutorials/video/colour-palette-and-paint

I just read the #962, that's a very interesting idea, like a separate color palette just for vector layers. We can seriously consider implementing it.

@CandyFace

This comment has been minimized.

Member

CandyFace commented May 20, 2018

It's indeed a feature in Toon Boom harmony.
A few of the differences I consider for our version is that we keep stroke and fill in the same window, and when we lay a stroke on the canvas of different color, it will be added automatically to the list instead of the user having to add it manually.

it should be possible to add a "disconnected" stroke again, but the default behaviour imo. should be that it adds the stroke and color automatically.

@CandyFace

This comment has been minimized.

Member

CandyFace commented May 26, 2018

Is there anything you're missing from this PR @chchwy or can we merge?

I will implement the "vector palette" possibly as part of 0.6.3, so this should be an alright solution until then.

connect(ui->colorListWidget, &QListWidget::itemClicked, this, &ColorPaletteWidget::clickColorListItem);
connect(ui->colorListWidget, &QListWidget::itemDoubleClicked, this, &ColorPaletteWidget::changeColourName);
connect(ui->colorListWidget, &QListWidget::currentTextChanged, this, &ColorPaletteWidget::onActiveColorNameChange);
connect(ui->addColorButton, &QPushButton::clicked, this, &ColorPaletteWidget::clickAddColorButton);
connect(ui->colorDialogButton, &QPushButton::clicked, this, &ColorPaletteWidget::clickColorDialogButton);
connect(ui->removeColorButton, &QPushButton::clicked, this, &ColorPaletteWidget::clickRemoveColorButton);
connect(ui->colorListWidget, &QListWidget::itemSelectionChanged, this, &ColorPaletteWidget::selectedItems);

This comment has been minimized.

@chchwy

chchwy May 30, 2018

Member

Is this connect() necessary? in selectedItems() it seems doing nothing.

This comment has been minimized.

@CandyFace

CandyFace May 30, 2018

Member

Ah that's a remnant from earlier testing, will remove

@@ -17,16 +17,20 @@ GNU General Public License for more details.
#include "colorpalettewidget.h"
#include "ui_colorpalette.h"
#include <cmath>
// Standard libraries
#include "cmath"

This comment has been minimized.

@chchwy

chchwy May 30, 2018

Member

minor thing, but usually system header uses angle brackets.

This comment has been minimized.

@CandyFace

CandyFace May 30, 2018

Member

Ah right, a typo. It should indeed be angle brackets.

// in cases where the last color was saved
// otherwise first color will be black.
editor()->color()->setColorNumber(0);
editor()->color()->setColor(editor()->color()->frontColor());

This comment has been minimized.

@chchwy

chchwy May 30, 2018

Member

These two lines should probably not be here. Usually in the initialization state, all the current states like the current frame, color, layer will be initialized in the editor & managers, UIs can only pull the states and updates itself. Not to change the states.

This comment has been minimized.

@CandyFace

CandyFace May 31, 2018

Member

Hmm it was necessary in the beginning, because otherwise it wouldn't save the previous color, except when you clicked on one of the swatches in the palette. It doesn't seem to be the case anymore though... so I'm removing it

@@ -51,3 +53,12 @@ bool ColourRef::operator!=(ColourRef colourRef1)
return false;
}
}
QDebug operator<<(QDebug debug, const ColourRef& colourRef)

This comment has been minimized.

@chchwy

chchwy May 30, 2018

Member

Another minor thing, I guess the return type would be better to be QDebug& not QDebug

@@ -82,10 +82,14 @@ class Object : public QObject
QString copyFileToDataFolder( QString strFilePath );
// Color palette
ColourRef getColour( int i ) const;
ColourRef getColour( int index ) const;
void useAsTempPaletteColor(QColor color) { mFrontColor = ColourRef(color, "Front Color"); }

This comment has been minimized.

@chchwy

chchwy May 30, 2018

Member

Could you please explain to me why adding useAsTempPaletteColor()?

This comment has been minimized.

@CandyFace

CandyFace May 30, 2018

Member

Because since vector is dependant on mPalette, if there are no swatches left, the color will become black. This makes sure that it'll use the front color instead if that's the case.
Do you want me to add it to Object::setColour instead?

The other solution is to not allow all swatches to be deleted but I find this to be a workaround rather than a proper solution.

This comment has been minimized.

@chchwy

chchwy Jun 5, 2018

Member

Probably the best solution for now (I mean before you implement the connected color), we couldn't allow users to delete all palette colors, need to at least keep one color.

This comment has been minimized.

@CandyFace

CandyFace Jun 5, 2018

Member

Okay so i'll remove the useTemp solution and make it impossible to delete the last remaining swatch.

This comment has been minimized.

@chchwy

chchwy Jun 5, 2018

Member

Thanks :)

}
else if (msgBox.clickedButton() == removeButton)
{
return true;
}

This comment has been minimized.

@chchwy

chchwy May 30, 2018

Member

It's probably not a good idea to pop up a message box in the Object, Object should be pure logic object. It breaks the principle of separating UI & logic and also makes writing unit tests harder as message box will block the function call until someone hits the button.

I would recommend to have a bool isColourInUse() and a separate void removeColor(). And the moving the message box to ColorPaletteWidget.

@chchwy

This comment has been minimized.

Member

chchwy commented May 30, 2018

Sorry for the late reply. I was sick and had two days in the bed.

CandyFace added some commits May 31, 2018

reverse temp color solution and fix a bug.
A warning will now be shown when the user tries to delete the last
remaining swatch
@CandyFace

This comment has been minimized.

Member

CandyFace commented Jun 11, 2018

Changes implemented, you can now review again and hopefully merge :)

@chchwy

This comment has been minimized.

Member

chchwy commented Jun 12, 2018

Thanks @CandyFace, well done.

@chchwy chchwy merged commit 2d08e9f into pencil2d:master Jun 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment