New Palette Docker for Krita (T8646)
AbandonedPublic

Authored by woltherav on Aug 14 2018, 12:12 AM.

Details

Summary

A New Palette Docker for Krita

This is a Google Summer of Code project.

Test Manual

Task:

https://phabricator.kde.org/T8646

The proposal is on the task page.

Brief Introduction

The new palette docker is designed and written to improve artists' experience of color management when using Krita.

The original palette docker enables users to create new palettes that are shared with all works and to maintain those palettes as a list of colors, though they are shown as matrices of colors.

Compared to the original palette docker, the new one is better mainly in these 3 ways:

  • The colors are now not only shown, but also actually managed in matrices. Users can move them around freely in those matrices. They can leave empty slots or let the colors from some pattern if they want.
  • The palettes are associated with the .kra painting documents. Artists can create temporary palettes for a single file but don't need to worry about polluting the environment when editing other paintings. If they want to reuse palettes, they can also set the palettes to be 'global'.
  • The docker now comes with a more intuitive GUI. Artists can now better edit the palettes and easily export and import them.

Merged and not Merged

In the early phase of GSoC, I noticed that some base classes of the palettes can be reused in other places. Therefore, I did some refactoring to reuse them. The code reviews are here:

https://phabricator.kde.org/D13750

https://phabricator.kde.org/D13208

These 2 code reviews got merged, however, most of my work are in this code review, not yet merged.

https://phabricator.kde.org/D14815

That is because merging parts of the change will break some functions. This project involves several widgets that cooperate with each other. If only some of the widgets got merged, master will have widgets that don't work with each other well. Therefore, we decided to merge after everything is done.

Details of usage

Each palette have the following attributes:

NameMeaning
Column numberNumber of columns.
NameName of the palette.
FilenamePath to where the palette is stored. For non-global palettes it is just a file name.
Is globalA palette is global if it isn't stored inside a .kra file, and can be accessed by all documents.
Is read onlyWhether a palette is read only. Palettes that ship with Krita are usually read only.

Also, each palette can have more than one groups. Each group has the same column number as the palette itself, and they each have a row number.

Every palette always has a main group that has no name. This group cannot be renamed or removed. One can set the row number of it to 0, though.

Below is a picture of the docker.

By clicking the Edit this palette button on the palette docker, one can open a dialog. In the dialog, one can set the attributes of the palette and the groups.

Each swatch in the palette docker also have more attributes than position and color. They each can have a name and string id. I actually don't know what the id is for... By double clicking a swatch, a dialog will pop up and let users edit it. One can also call the dialog by clicking the modify this spot button on the docker.

The docker also supports drag and drop. By dragging and dropping one can move a swatch to a new position or switch it with another swatch.

On the bottom left corner of the docker, there is a pop up button. After clicking it, the palette list widget will pop up.

At the bottom of the palette list widget, there are 4 buttons. From left to right, they are respectively Add a new palette, Export current palette to file, Import a new palette from file, and Remove current palette. By default, Add and Import create non-global palettes. Remove can remove all palettes, global or non-global, except for those read only ones.

Done and To Do

All the features that are in the proposal are done. This involves the palette docker itself, the base classes in the underlying libraries relating to it, and other classes using these base classes. There are probably still some bugs, so I will need to work on locating and solving them.

While working on the palette docker, I got some new ideas, and also found some problems in the code base that I want to solve. Those include:

  • Making it possible to select multiple swatches in the palette so that they can be drag/dropped or removed.
  • Making it possible to drag the group names rows in the palette docker to reorder them.
  • Use a list view (instead of a combo box) to show the groups in the dialog used to edit them, and making it easier to rename, delete and reorder groups. It should be like the layers docker.
  • Use KisDlgInternalColorSelector to replace KoColorSetWidget. This way, the dependency that holds palette classes in kritawidgets will be resolved and it can be moved up into kritaui, and have some better tools to use.
  • Improve KisDlgInternalColorSelector, making the triangular selector more smooth and the spin boxes show color values normally.

Details of what has been done

KoColorSet and KisPaletteModel have been heavily modified. Along with the new classes KisSwatch and KisSwatchGroup, they form the underlying data structure of palettes, which are grouped color matrices.

The API of KoColorSet and KisPaletteModel have been modified. What I want is to let all the data manipulation be done in KisPaletteModel. This is not done yet.

Surely, KisPaletteView and KisPaletteDelegate are modified to show the new matrices of colors.

The above group form the MVC system of palettes.

For making the modification of the palette tell krita that the document has changed, KisChangePaletteCommand inheriting from KUndo2Command is created.

For managing the list of palettes, KisPaletteListWidget is created to replace KisColorsetChooser.

For the search box in the palette docker and the internal color selector, KisPaletteComboBox is created. It syncs with a KisPaletteView assigned to it.

Due the change in API and usage of classes, other classes that use the palettes are also affected. Those classes include KoColorSetWidget, KisDlgInternalColorSelector, KoEditColorSetDialog, KisToolLazyBrushOptionsWidget, and LayerSplit.

KoEditColorSetDialog is removed because its functions can totally be replaced by the palette docker. KoColorSetWidget is planned to be replaced by KisDlgInternalColorSelector. LayerSplit and KisToolLazyBrushOptionsWidget are modified to adapt to the new API and tools.

In order to make it possible to store palettes in .kra, KisDocument is modified. It have a new member called paletteList, a list of pointers to palettes that belong to it. In KoColorSet, toByteArray() and fromByteArray() are implemented so that it can save itself into a buffer and then be transferred and stored into .kra.

Inside the docker, the setCanvas() and unsetCanvas() callbacks manipulates the resources server following the paletteList of the document being opened and closed.

.kpl

Original .kpl: https://phabricator.kde.org/T4121

An example for the new colorset.xml in the .kpl:

<ColorSet columns="16" comment="" readonly="false" rows="3" version="1.0" name="PALETTE">
 <ColorSetEntry bitdepth="U8" id="" spot="false" name="hair bright light">
  <RGB g="0.666666686534882" space="sRGB-elle-V2-srgbtrc.icc" r="0.898039221763611" b="0.549019634723663"/>
  <Position column="0" row="0"/>
 </ColorSetEntry>
 <ColorSetEntry bitdepth="U8" id="" spot="false" name="hair 1">
  <RGB g="0.313725501298904" space="sRGB-elle-V2-srgbtrc.icc" r="0.501960813999176" b="0.239215686917305"/>
  <Position column="1" row="0"/>
 </ColorSetEntry>
 <ColorSetEntry bitdepth="U8" id="" spot="false" name="hair mid">
  <RGB g="0.321568638086319" space="sRGB-elle-V2-srgbtrc.icc" r="0.533333361148834" b="0.250980406999588"/>
  <Position column="2" row="0"/>
 </ColorSetEntry>
 <ColorSetEntry bitdepth="U8" id="" spot="false" name="hair dark">
  <RGB g="0.129411771893501" space="sRGB-elle-V2-srgbtrc.icc" r="0.30588236451149" b="0.109803922474384"/>
  <Position column="3" row="0"/>
 </ColorSetEntry>
 <ColorSetEntry bitdepth="U8" id="" spot="false" name="hair 3">
  <RGB g="0.211764708161354" space="sRGB-elle-V2-srgbtrc.icc" r="0.380392163991928" b="0.18823529779911"/>
  <Position column="4" row="0"/>
 </ColorSetEntry>
 <Group rows="3" name="skin">
  <ColorSetEntry bitdepth="U8" id="" spot="false" name="skin 0">
   <RGB g="0.992156863212585" space="sRGB-elle-V2-srgbtrc.icc" r="1" b="0.925490200519562"/>
   <Position column="0" row="0"/>
  </ColorSetEntry>
  <ColorSetEntry bitdepth="U8" id="" spot="false" name="skin 1">
   <RGB g="0.949019610881805" space="sRGB-elle-V2-srgbtrc.icc" r="1" b="0.874509811401367"/>
   <Position column="1" row="0"/>
  </ColorSetEntry>
  <ColorSetEntry bitdepth="U8" id="" spot="false" name="skin 2">
   <RGB g="0.82745099067688" space="sRGB-elle-V2-srgbtrc.icc" r="0.941176474094391" b="0.796078443527222"/>
   <Position column="2" row="0"/>
  </ColorSetEntry>
  <ColorSetEntry bitdepth="U8" id="" spot="false" name="skin 4">
   <RGB g="0.65490198135376" space="sRGB-elle-V2-srgbtrc.icc" r="0.819607853889465" b="0.619607865810394"/>
   <Position column="3" row="0"/>
  </ColorSetEntry>
 </Group>
</ColorSet>

Following are the differences from the original .kpl:

The new rows and readonly attribute in the ColorSet tag. rows here means the row number of the main group. This attribute also exists and have the same meaning in the Group tags.

A Position tag now exists for each ColorSetEntry. It decides the position of the swatch defined by the ColorSetEntry tag.

Other Related links

Blog posts:
https://simeir.github.io/kde/2018/05/13/First-Post.html
https://simeir.github.io/kde/2018/06/22/Second-Post.html
https://simeir.github.io/kde/2018/06/29/Third-Post.html
https://simeir.github.io/kde/2018/07/13/Fifth-Post.html

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
mzhou requested review of this revision.Aug 14 2018, 12:12 AM
mzhou created this revision.
mzhou retitled this revision from New Palette Docker for Krita to New Palette Docker for Krita (T8646).
mzhou edited the summary of this revision. (Show Details)Aug 14 2018, 12:43 AM
mzhou edited the summary of this revision. (Show Details)Aug 14 2018, 1:29 AM
mzhou edited the summary of this revision. (Show Details)Aug 14 2018, 1:32 AM
mzhou edited the summary of this revision. (Show Details)
mzhou edited the summary of this revision. (Show Details)Aug 14 2018, 2:25 AM
mzhou edited the summary of this revision. (Show Details)
mzhou edited the summary of this revision. (Show Details)Aug 14 2018, 2:30 AM
mzhou edited the summary of this revision. (Show Details)Aug 14 2018, 2:32 AM
mzhou edited the summary of this revision. (Show Details)Aug 14 2018, 2:54 AM
mzhou edited the summary of this revision. (Show Details)Aug 14 2018, 2:58 AM
mzhou edited the summary of this revision. (Show Details)Aug 14 2018, 3:06 AM
mzhou edited the summary of this revision. (Show Details)Aug 14 2018, 3:09 AM
mzhou updated this revision to Diff 39661.Aug 14 2018, 5:24 AM

Fixed a bug with the lazy brush options widget

mzhou updated this revision to Diff 39662.Aug 14 2018, 5:54 AM

Selection won't be made when empty slot is selected

mzhou updated this revision to Diff 39663.Aug 14 2018, 6:00 AM
mzhou updated this revision to Diff 39702.Aug 14 2018, 2:25 PM
scottpetrovic added a subscriber: scottpetrovic.EditedAug 16 2018, 11:34 PM

Is this the same code that is in your branch? I am getting a build error when trying to build the branch...

/src/libs/pigment/tests/TestKisSwatchGroup.cpp:27: undefined reference to `bool QTest::qCompare<QString, char [6]>(QString const&, char const (&) [6], char const*, char const*, char const*, int)'
/home/scott/krita/src/libs/pigment/tests/TestKisSwatchGroup.cpp:33: undefined reference to `bool QTest::qCompare<QString, char [7]>(QString const&, char const (&) [7], char const*, char const*, char const*, int)'
CMakeFiles/TestKisSwatchGroup.dir/TestKisSwatchGroup.cpp.o: In function `TestKisSwatchGroup::testReplaceEntries()':
/home/scott/krita/src/libs/pigment/tests/TestKisSwatchGroup.cpp:45: undefined reference to `bool QTest::qCompare<QString, char [7]>(QString const&, char const (&) [7], char const*, char const*, char const*, int)'
collect2: error: ld returned 1 exit status

I wiped out my build directory to do a clean build and it still gave that error

mzhou added a comment.EditedAug 17 2018, 1:41 AM

Is this the same code that is in your branch? I am getting a build error when trying to build the branch...

/src/libs/pigment/tests/TestKisSwatchGroup.cpp:27: undefined reference to `bool QTest::qCompare<QString, char [6]>(QString const&, char const (&) [6], char const*, char const*, char const*, int)'
/home/scott/krita/src/libs/pigment/tests/TestKisSwatchGroup.cpp:33: undefined reference to `bool QTest::qCompare<QString, char [7]>(QString const&, char const (&) [7], char const*, char const*, char const*, int)'
CMakeFiles/TestKisSwatchGroup.dir/TestKisSwatchGroup.cpp.o: In function `TestKisSwatchGroup::testReplaceEntries()':
/home/scott/krita/src/libs/pigment/tests/TestKisSwatchGroup.cpp:45: undefined reference to `bool QTest::qCompare<QString, char [7]>(QString const&, char const (&) [7], char const*, char const*, char const*, int)'
collect2: error: ld returned 1 exit status

I wiped out my build directory to do a clean build and it still gave that error

No, the branch contains some more fixes. I will update the diff after this comment.

I didn't get that build error myself. I guess this build error is caused because you were compiliing using Qt 4. QCOMPARE is strict on types in Qt 4 while it isn't in Qt 5.

I just push a commit in which the type difference is fixed. I don't know if that is going to work though, because I don't know how to configure cmake to use Qt 4... Can you tell me how to do that?

mzhou updated this revision to Diff 39894.Aug 17 2018, 1:47 AM

Changed TestKisSwatchGroup so that all QCOMPARE have values with the same type

mzhou edited the summary of this revision. (Show Details)Aug 20 2018, 9:14 AM
rempt added a subscriber: rempt.Aug 20 2018, 10:06 AM

Everything seems to work.

There is some work to be done, though, when it comes to usability. The dialog doesn't feel very comfortable. But that's something for Scott to look into.

Another thing: I saved a copy of concept-cookie.gpl to the kra file. That worked fine. I was a bit surprised that it was saved in .kpl format, not .gpl format. I guess that when saving palettes to kra files, the extension always should be changed to .kpl.

I'm still testing, but in general, I'm very happy with the work done.

dkazakov requested changes to this revision.Aug 20 2018, 11:51 AM

Hi, @mzhou!

There is some invalid memory access in the palettes docker, so I cannot even start Krita in a build with memory sanitizer on. Here is the output:

appimage@laptop5:/home/appimage/appimage-workspace/krita-build>ASAN_OPTIONS=symbolize=1 krita
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-appimage'
KoColor debug runtime checks are active.
process 1457: D-Bus library appears to be incorrectly set up; failed to read machine uuid: Failed to open "/etc/machine-id": No such file or directory
See the manual page for dbus-uuidgen to correct this issue.
QLayout: Attempting to add QLayout "" to QWidget "", which already has a layout
=================================================================
==1457== ERROR: AddressSanitizer: heap-use-after-free on address 0x600602b1a1d0 at pc 0x7f2fa692d4ad bp 0x7ffc6fe3fc60 sp 0x7ffc6fe3fc58
READ of size 4 at 0x600602b1a1d0 thread T0
    #0 0x7f2fa692d4ac in KisPaletteModel::rowNumberInGroup(int) const /home/appimage/persistent/krita/libs/widgets/KisPaletteModel.cpp:135
    #1 0x7f2fa692db8e in KisPaletteModel::dataForSwatch(QModelIndex const&, int) const /home/appimage/persistent/krita/libs/widgets/KisPaletteModel.cpp:422
    #2 0x7f2fa692ea3b in KisPaletteModel::data(QModelIndex const&, int) const /home/appimage/persistent/krita/libs/widgets/KisPaletteModel.cpp:56
    #3 0x7f2fa695ab8b in QModelIndex::data(int) const /home/appimage/appimage-workspace/deps/usr/include/QtCore/qabstractitemmodel.h:432
    #4 0x7f2fa695ab8b in KisPaletteComboBox::slotSwatchSelected(QModelIndex const&) /home/appimage/persistent/krita/libs/widgets/KisPaletteComboBox.cpp:146
    #5 0x7f2fa699ea38 in KisPaletteComboBox::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/appimage/appimage-workspace/krita-build/libs/widgets/moc_KisPaletteComboBox.cpp:96
    #6 0x7f2faa97d1de (/home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5+0x2a61de)
    #7 0x7f2fa69902ad in KisPaletteView::sigIndexSelected(QModelIndex const&) /home/appimage/appimage-workspace/krita-build/libs/widgets/moc_kis_palette_view.cpp:183
    #8 0x7f2fa6941181 in KisPaletteView::slotCurrentSelectionChanged(QModelIndex const&) /home/appimage/persistent/krita/libs/widgets/kis_palette_view.cpp:267
    #9 0x7f2fa6999db0 in KisPaletteView::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/appimage/appimage-workspace/krita-build/libs/widgets/moc_kis_palette_view.cpp:103
    #10 0x7f2faa97d1de (/home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5+0x2a61de)
    #11 0x7f2faa90b849 (/home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5+0x234849)
    #12 0x7f2faa90bb1d (/home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5+0x234b1d)
    #13 0x7f2fa6940781 in KisPaletteView::selectClosestColor(KoColor const&) /home/appimage/persistent/krita/libs/widgets/kis_palette_view.cpp:194
    #14 0x7f2fa6921295 in KisDlgInternalColorSelector::updateAllElements(QObject*) /home/appimage/persistent/krita/libs/widgets/KisDlgInternalColorSelector.cpp:287
    #15 0x7f2fadb33988 in KoDualColorButton::setForegroundColor(KoColor const&) /home/appimage/persistent/krita/libs/ui/widgets/KoDualColorButton.cpp:163
    #16 0x7f2fae01df5b in KoDualColorButton::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/appimage/appimage-workspace/krita-build/libs/ui/moc_KoDualColorButton.cpp:143
    #17 0x7f2faa97d1de (/home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5+0x2a61de)
    #18 0x7f2fadffa5bd in KisCanvasResourceProvider::sigFGColorChanged(KoColor const&) /home/appimage/appimage-workspace/krita-build/libs/ui/moc_kis_canvas_resource_provider.cpp:410
    #19 0x7f2fad40587e in KisCanvasResourceProvider::slotCanvasResourceChanged(int, QVariant const&) /home/appimage/persistent/krita/libs/ui/kis_canvas_resource_provider.cpp:303
    #20 0x7f2fae01c7de in KisCanvasResourceProvider::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/appimage/appimage-workspace/krita-build/libs/ui/moc_kis_canvas_resource_provider.cpp:211
    #21 0x7f2faa97d1de (/home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5+0x2a61de)
    #22 0x7f2fa6127b0b in KoCanvasResourceManager::canvasResourceChanged(int, QVariant const&) /home/appimage/appimage-workspace/krita-build/libs/flake/moc_KoCanvasResourceManager.cpp:153
    #23 0x7f2faa97d882 (/home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5+0x2a6882)
    #24 0x7f2fa6127d8b in KoResourceManager::resourceChanged(int, QVariant const&) /home/appimage/appimage-workspace/krita-build/libs/flake/moc_KoResourceManager_p.cpp:137
    #25 0x7f2fa5c47120 in KoResourceManager::notifyResourceChanged(int, QVariant const&) /home/appimage/persistent/krita/libs/flake/KoResourceManager_p.cpp:74
    #26 0x7f2fa5c49808 in KoResourceManager::setResource(int, QVariant const&) /home/appimage/persistent/krita/libs/flake/KoResourceManager_p.cpp:67
    #27 0x7f2fad3fb9e3 in KisCanvasResourceProvider::setFGColor(KoColor const&) /home/appimage/persistent/krita/libs/ui/kis_canvas_resource_provider.cpp:230
    #28 0x7f2fade32f05 in KisViewManager::KisViewManager(QWidget*, KActionCollection*) /home/appimage/persistent/krita/libs/ui/KisViewManager.cpp:319
    #29 0x7f2fadd9b81f in KisMainWindow::KisMainWindow(QUuid) /home/appimage/persistent/krita/libs/ui/KisMainWindow.cpp:307
    #30 0x7f2faddc196f in KisPart::createMainWindow(QUuid) /home/appimage/persistent/krita/libs/ui/KisPart.cpp:217
    #31 0x7f2faddc21c3 in KisPart::startBlankSession() /home/appimage/persistent/krita/libs/ui/KisPart.cpp:546
    #32 0x7f2fadcf7496 in KisApplication::start(KisApplicationArguments const&) /home/appimage/persistent/krita/libs/ui/KisApplication.cpp:454
    #33 0x406e94 in main /home/appimage/persistent/krita/krita/main.cc:420
    #34 0x7f2fa9e15f44 (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
    #35 0x40caed in _start (/home/appimage/appimage-workspace/krita-inst/bin/krita+0x40caed)
0x600602b1a1d0 is located 16 bytes inside of 24-byte region [0x600602b1a1c0,0x600602b1a1d8)
freed by thread T0 here:
    #0 0x7f2fb038e33a (/usr/lib/x86_64-linux-gnu/libasan.so.0+0x1533a)
    #1 0x7f2fa692d29e in QList<int>::dealloc(QListData::Data*) /home/appimage/appimage-workspace/deps/usr/include/QtCore/qlist.h:867
    #2 0x7f2fa692d29e in ~QList /home/appimage/appimage-workspace/deps/usr/include/QtCore/qlist.h:827
    #3 0x7f2fa692d29e in KisPaletteModel::rowNumberInGroup(int) const /home/appimage/persistent/krita/libs/widgets/KisPaletteModel.cpp:134
previously allocated by thread T0 here:
    #0 0x7f2fb038e41a (/usr/lib/x86_64-linux-gnu/libasan.so.0+0x1541a)
    #1 0x7f2faa7d761b (/home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5+0x10061b)
SUMMARY: AddressSanitizer: heap-use-after-free /home/appimage/persistent/krita/libs/widgets/KisPaletteModel.cpp:135 KisPaletteModel::rowNumberInGroup(int) const
Shadow bytes around the buggy address:
  0x0c014055b3e0: fd fd fa fa fd fd fd fd fa fa fd fd fd fd fa fa
  0x0c014055b3f0: fd fd fd fd fa fa fd fd fd fa fa fa fa fa fa fa
  0x0c014055b400: fa fa fd fd fd fd fa fa fd fd fd fd fa fa fd fd
  0x0c014055b410: fd fd fa fa fd fd fd fd fa fa fd fd fd fd fa fa
  0x0c014055b420: fd fd fd fa fa fa fd fd fd fd fa fa fa fa fa fa
=>0x0c014055b430: fa fa fd fd fd fa fa fa fd fd[fd]fa fa fa 00 00
  0x0c014055b440: 00 00 fa fa fd fd fd fd fa fa fd fd fd fd fa fa
  0x0c014055b450: 00 00 00 00 fa fa fd fd fd fa fa fa fa fa fa fa
  0x0c014055b460: fa fa fd fd fd fd fa fa fd fd fd fd fa fa fd fd
  0x0c014055b470: fd fd fa fa fd fd fd fd fa fa fd fd fd fd fa fa
  0x0c014055b480: fd fd fd fa fa fa fd fd fd fd fa fa 00 00 00 fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==1457== ABORTING
appimage@laptop5:/home/appimage/appimage-workspace/krita-build>
This revision now requires changes to proceed.Aug 20 2018, 11:51 AM
dkazakov added a comment.EditedAug 20 2018, 11:59 AM

UPD: I've found out that I can push the fix directly into the branch. So now it is pushed

Here is the fix for that memory access problem: the two iterators are created from two different temporary objects returned from the 'keys()' method.

1diff --git a/libs/widgets/KisPaletteModel.cpp b/libs/widgets/KisPaletteModel.cpp
2index 16d8da9..76d2a60 100644
3--- a/libs/widgets/KisPaletteModel.cpp
4+++ b/libs/widgets/KisPaletteModel.cpp
5@@ -131,7 +131,9 @@ int KisPaletteModel::rowNumberInGroup(int rowInModel) const
6 if (m_groupNameRows.contains(rowInModel)) {
7 return -1;
8 }
9- for (auto it = m_groupNameRows.keys().rbegin(); it != m_groupNameRows.keys().rend(); it++) {
10+
11+ auto keys = m_groupNameRows.keys();
12+ for (auto it = keys.rbegin(); it != keys.rend(); it++) {
13 if (*it < rowInModel) {
14 return rowInModel - *it - 1;
15 }

UPD: I've found out that I can push the fix directly into the branch. So now it is pushed

Here is the fix for that memory access problem: the two iterators are created from two different temporary objects returned from the 'keys()' method.

1diff --git a/libs/widgets/KisPaletteModel.cpp b/libs/widgets/KisPaletteModel.cpp
2index 16d8da9..76d2a60 100644
3--- a/libs/widgets/KisPaletteModel.cpp
4+++ b/libs/widgets/KisPaletteModel.cpp
5@@ -131,7 +131,9 @@ int KisPaletteModel::rowNumberInGroup(int rowInModel) const
6 if (m_groupNameRows.contains(rowInModel)) {
7 return -1;
8 }
9- for (auto it = m_groupNameRows.keys().rbegin(); it != m_groupNameRows.keys().rend(); it++) {
10+
11+ auto keys = m_groupNameRows.keys();
12+ for (auto it = keys.rbegin(); it != keys.rend(); it++) {
13 if (*it < rowInModel) {
14 return rowInModel - *it - 1;
15 }

Just saw it. Thanks!

mzhou updated this revision to Diff 40055.Aug 20 2018, 2:07 PM

Merged Dmitry's fix

dkazakov requested changes to this revision.Aug 21 2018, 8:03 AM

Hi, @mzhou!

I have tested your branch. Basically, it works fine, although I have found a few issues of different severity:

  1. [major] When one changes the palette, an undo command is created. But undoing this undo command does nothing. Looking into the code, it looks like it is not even implemented.
  2. [major] Undo commands produced by the palette are not compressed in the undo stack. To let compression work, one should override and implement mergeWith() method inside the command.
  3. [major] After making a palette read-only, one cannot make it writable again. All the buttons are disabled, so you cannot even delete this palette. It will keep in the list forever :)
  4. [uix, minor] When a palette is read-only, "Delete spot" button is still active. Although clicking on it does nothing.
  5. [uix, major] When creating a new palette in a palette chooser popup, one cannot rename the newly created palette. There should either be a delegate in the palettes list that would allow renaming, or just a simple button near import/export/delete buttons.
  6. [uix, minor] Double-clicking on a popup palette selector doesn't close it. It is quite nasty, because sometimes the popup overlaps the palette widget itself and you cannot see, did your click succeeded or not.
  7. [uix, minor] Default folder for importing/exporting the palettes should point either to a home directory or the resources folder. Right now it looks like no argument is passed to it, therefore it points just to the folder, where Krita binary is. That is very user-unfriendly behavior.
  8. [uix, minor, disputable] If the user saves .kra document and some non-global palette is active, it would be nice to save the name of this palette to .kra, so that it would be automatically activated on loading this document. But this very point can be implemented after the merge.
This revision now requires changes to proceed.Aug 21 2018, 8:03 AM
rempt added a comment.Oct 4 2018, 11:06 AM

I see a couple of problems still:

  • Changed palettes are saved as .kpl, but with a .gpl extension so they cannot be loaded
  • Column count can get set to 1
  • global checkbox does not make clear whether the palette is system installed, locally modified or part of the kra document
woltherav added a subscriber: woltherav.EditedOct 4 2018, 12:40 PM

I've fixed the saving of the kpl files, as well as turned global into a buttongroup that can switch between 'resource folder'(global) and 'document'.

  • Column thing. Can't reproduce.
  • Making a document palette global doesn't store it in the resource folder.
woltherav added a comment.EditedOct 4 2018, 7:54 PM

Found an Assert :
If you have a palette, and you reduce the amount of columns to below 4 of columns, we get the following assert:

ASSERT failure in QVector<T>::operator[]: "index out of range", file /usr/include/x86_64-linux-gnu/qt5/QtCore/qvector.h, line 436

Thread 1 (Thread 0x7ffff7f92cc0 (LWP 22527)):
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff3eca801 in __GI_abort () at abort.c:79
#2  0x00007ffff489c5db in QMessageLogger::fatal(char const*, ...) const () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#3  0x00007ffff489be2b in qt_assert_x(char const*, char const*, char const*, int) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#4  0x00007ffff06f1bf1 in QVector<QMap<int, KisSwatch> >::operator[] (this=0x55555bfc13a8, i=-1)
    at /usr/include/x86_64-linux-gnu/qt5/QtCore/qvector.h:436
#5  0x00007ffff06f0cef in KisSwatchGroup::setColumnCount (this=0x55555bf4cf38, columnCount=4)
    at /home/wolthera/krita/src/libs/pigment/resources/KisSwatchGroup.cpp:113
---Type <return> to continue, or q <return> to quit---
#6  0x00007ffff06dc455 in KoColorSet::setColumnCount (this=0x55555c0afda0, columns=4)
    at /home/wolthera/krita/src/libs/pigment/resources/KoColorSet.cpp:318
#7  0x00007ffff7217b68 in KisPaletteEditor::updatePalette (this=0x55556cdff560)
    at /home/wolthera/krita/src/libs/ui/KisPaletteEditor.cpp:474
#8  0x00007ffff7221984 in KisDlgPaletteEditor::slotAccepted (this=0x7fffffffd110)
    at /home/wolthera/krita/src/libs/ui/dialogs/KisDlgPaletteEditor.cpp:235
#9  0x00007ffff73e332e in KisDlgPaletteEditor::qt_static_metacall (_o=0x7fffffffd110, _c=QMetaObject::InvokeMetaMethod, 
    _id=10, _a=0x7fffffffc1c0)
    at /home/wolthera/krita/build/libs/ui/kritaui_autogen/2NRMJ5X7RK/moc_KisDlgPaletteEditor.cpp:119
#10 0x00007ffff4ac6ad5 in QMetaObject::activate(QObject*, int, int, void**) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#11 0x00007ffff4ac6ad5 in QMetaObject::activate(QObject*, int, int, void**) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#12 0x00007ffff59add70 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#13 0x00007ffff4ac6ad5 in QMetaObject::activate(QObject*, int, int, void**) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#14 0x00007ffff590a0f2 in QAbstractButton::clicked(bool) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#15 0x00007ffff590a30a in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#16 0x00007ffff590b6ea in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#17 0x00007ffff590b8dd in QAbstractButton::mouseReleaseEvent(QMouseEvent*) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#18 0x00007ffff5863b08 in QWidget::event(QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#19 0x00007ffff5824e8c in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#20 0x00007ffff582cff7 in QApplication::notify(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#21 0x00007ffff72db8bf in KisApplication::notify (this=0x7fffffffe100, receiver=0x5555701e1520, event=0x7fffffffc990)
    at /home/wolthera/krita/src/libs/ui/KisApplication.cpp:610
#22 0x00007ffff4a97ab8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#23 0x00007ffff582b942 in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#24 0x00007ffff587ec73 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#25 0x00007ffff5881289 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#26 0x00007ffff5824e8c in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
---Type <return> to continue, or q <return> to quit---
#27 0x00007ffff582c45f in QApplication::notify(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#28 0x00007ffff72db8bf in KisApplication::notify (this=0x7fffffffe100, receiver=0x55556cad67a0, event=0x7fffffffce20)
    at /home/wolthera/krita/src/libs/ui/KisApplication.cpp:610
#29 0x00007ffff4a97ab8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#30 0x00007ffff50292ab in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
#31 0x00007ffff502ae55 in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
#32 0x00007ffff5003f0b in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
#33 0x00007fffde56bedb in ?? () from /usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5
#34 0x00007ffff4a95dea in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#35 0x00007ffff5a13ba7 in QDialog::exec() () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#36 0x00007fffb68fa7a8 in PaletteDockerDock::slotEditPalette (this=0x55556793ac40)
    at /home/wolthera/krita/src/plugins/dockers/palettedocker/palettedocker_dock.cpp:275
#37 0x00007fffb6901feb in PaletteDockerDock::qt_static_metacall (_o=0x55556793ac40, _c=QMetaObject::InvokeMetaMethod, 
    _id=8, _a=0x7fffffffd330)
    at /home/wolthera/krita/build/plugins/dockers/palettedocker/kritapalettedocker_autogen/EWIEGA46WW/moc_palettedocker_dock.cpp:143
#38 0x00007ffff4ac6ad5 in QMetaObject::activate(QObject*, int, int, void**) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#39 0x00007ffff581e732 in QAction::triggered(bool) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#40 0x00007ffff5820d6c in QAction::activate(QAction::ActionEvent) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#41 0x00007ffff590b67b in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#42 0x00007ffff590b8dd in QAbstractButton::mouseReleaseEvent(QMouseEvent*) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#43 0x00007ffff59fa51a in QToolButton::mouseReleaseEvent(QMouseEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#44 0x00007ffff5863b08 in QWidget::event(QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#45 0x00007ffff59fa5b4 in QToolButton::event(QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#46 0x00007ffff5824e8c in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#47 0x00007ffff582cff7 in QApplication::notify(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#48 0x00007ffff72db8bf in KisApplication::notify (this=0x7fffffffe100, receiver=0x555567a08de0, event=0x7fffffffd8a0)
---Type <return> to continue, or q <return> to quit---
    at /home/wolthera/krita/src/libs/ui/KisApplication.cpp:610
#49 0x00007ffff4a97ab8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#50 0x00007ffff582b942 in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#51 0x00007ffff587ec73 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#52 0x00007ffff5881289 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#53 0x00007ffff5824e8c in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#54 0x00007ffff582c45f in QApplication::notify(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#55 0x00007ffff72db8bf in KisApplication::notify (this=0x7fffffffe100, receiver=0x5555642b5890, event=0x7fffffffdd30)
    at /home/wolthera/krita/src/libs/ui/KisApplication.cpp:610
#56 0x00007ffff4a97ab8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#57 0x00007ffff50292ab in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
#58 0x00007ffff502ae55 in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
#59 0x00007ffff5003f0b in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
#60 0x00007fffde56bedb in ?? () from /usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5
#61 0x00007ffff4a95dea in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#62 0x00007ffff4a9efa0 in QCoreApplication::exec() () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#63 0x0000555555e91587 in main (argc=1, argv=0x7fffffffe268) at /home/wolthera/krita/src/krita/main.cc:435
(gdb)

As this is an assert, it needs a Debug build, and not RelWithDebug.

woltherav added a comment.EditedOct 5 2018, 2:24 PM

So, I cannot reproduce the single column bug, and making a document palette unglobal now does save it to the resource folder(proly dolphin not updating).

Sometimes a palette saves as 0 bytes, leading to an assert. I've pushed an extra check against that, but not sure what caused it. @dkazakov, is there perhaps something odd with the copying going on?

The assert posted earlier only happens when you're trying to get less than 4 columns. When you decrease columns below the maximum existing swatches those swatches get removed without warning. EDIT: They don't get removed, just hidden.

woltherav added a comment.EditedOct 5 2018, 3:19 PM

Okay, so, I was looking a this with boud: We're not really sure how this all is supossed to work, starting with that the setColumn causing an assert, but we cannot figure out what said code is supossed to do(it reduces the total amount of colors, but not the entries themselves), and for some reason that KoColorSet::colorCount only returns the amount of colours for the default group, but nothing else?

Okay, we've fixed setColumn and I've fixed some other tiny things. What is needed now is that it gets tested.

woltherav commandeered this revision.Oct 6 2018, 3:50 PM
woltherav added a reviewer: mzhou.

Boud is currently merging this. We'll let artists test now :)

This revision now requires changes to proceed.Oct 6 2018, 4:13 PM
woltherav abandoned this revision.Oct 6 2018, 4:14 PM

Naw, this is the only option.

mzhou added a comment.Oct 6 2018, 5:52 PM

Naw, this is the only option.

Thank you so much for repairing this!