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.

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

Improve color palette selection behavior #1028

Merged
merged 2 commits into from Aug 2, 2018

Conversation

scribblemaniac
Copy link
Member

This includes:

  • Showing the proper icon color when selected (Fixes Selected color in Color Palette  #1024)
  • Adding a border around the selected color, this is particularly
    useful for grid mode which has no other way to display the selection
  • Refactored the resizing code so that going to grid mode from list
    mode does not result in unexpected icon spacing

This includes:
- Showing the proper icon color when selected (Fixes pencil2d#1024)
- Adding a border around the selected color, this is particularly
useful for grid mode which has no other way to display the selection
- Refactored the resizing code so that going to grid mode from list
mode does not result in unexpected icon spacing
swatchPainter.drawRect(0, 0, mIconSize.width() - 1, mIconSize.height() - 1);
swatchPainter.setPen(borderShadow);
swatchPainter.drawRect(0, 0, mIconSize.width() - 1, mIconSize.height() - 1);
swatchIcon.addPixmap(colourSwatch, QIcon::Selected);
Copy link
Member

Choose a reason for hiding this comment

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

Minor thing: selection border should imo. only be shown in grid mode. There's already normal cell selection highlight in list mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it isn't necessary in list mode, but I think it's still useful because the selection highlight differs based on the operating system. Here's a screenshot of the QListWidget from the qt docs:

Its style has much less emphasis on a selection than on a mac and a selection border may look better in styles like this.

Also I feel like it would be a bit of a pain to change because you would have to change the icons each time you toggle the mode, and either regenerate or store and retrieve the selection pixmap for each color when switching to grid mode. So part of the reason why I didn't do this is just laziness 😉

Copy link
Member

Choose a reason for hiding this comment

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

The list is updated on every click anyway so the required changes are minimal to hide the border. The style also seems pretty consistent across systems too, so aside from changing the Qt Theme from source code, the style should be somewhat consistent.

The image you provided is not what it looks like in Pencil2D though, regardless the operation system as far as I know, this is how it looks through wine:
image

The only changes that are required is adding if statements around your selection border painting. ~290 and ~650

Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason I was thinking that refreshColorList wasn't called on mode change, but I can see now that it is, and you're right it's very simple to change. I have pushed the changes needed to show the selection border in grid mode only.

@MrStevns
Copy link
Member

Here's what it looks like for the curious:
for the curious

Good job @scribblemaniac
I had started working on a similar solution but I found the icon selection states odd so I didn't quite get how to remove the highlight. I ended up making a custom item delegate but I see that's not necessary.

@scribblemaniac
Copy link
Member Author

I ended up making a custom item delegate but I see that's not necessary.

I spent some time trying to remove the highlight with stylesheets, and when that didn't work I was just about to try a custom item delegate when I stumbled upon this solution.

@chchwy
Copy link
Member

chchwy commented Aug 2, 2018

Cool! it's a nice solution. Thank you @scribblemaniac

@chchwy chchwy merged commit dd86ae4 into pencil2d:master Aug 2, 2018
@scribblemaniac scribblemaniac deleted the color-palette-select-fix branch August 21, 2018 04:12
@chchwy chchwy added this to the 0.6.2 milestone Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants