I copied all KoToolBox* file and KissKineticScroller (renamed to KoKineticScroller) from Krita repo.
I'm adding Krita as reviewer, because Karbon is unmaintained and I hope someone at Krita as some experience with the toolbox
Old:
New:
Details
Details
- Reviewers
anthonyfieroni - Group Reviewers
Calligra: 3.0 - Commits
- R8:a7ee6f871af2: Use Krita toolbar in Karbon
New:
Compile and run
Diff Detail
Diff Detail
- Repository
- R8 Calligra
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
I'm adding Krita as reviewer, because Karbon is unmaintained and I hope someone at Krita as some experience with the toolbox
I'm the maintainer of Karbon, for now.
Please revert foreach -> Q_FOREACH changes, it should be done in separate patch. Clean dead / commented code, white-space changes, don't remove QObject macro, unless you have a good reason for that. Override is good feature but in separate patch for old code, for new one it's good.
Back-porting patches from Krita are welcome but in accepted condition to other part of the project.
| libs/widgets/KoToolBoxDocker.cpp | ||
|---|---|---|
| 40 | Why empty or white-space label? | |
| libs/widgets/KoToolBoxLayout_p.h | ||
| 52 | It should be qCWarning, with category or remove it. | |
| 85 | Revert. | |
| 198–199 | Revert | |
| libs/widgetutils/KoKineticScroller.cpp | ||
| 27 | { on new line. | |
| 90 | { on new line. | |
Comment Actions
Good to know :D
Please revert foreach -> Q_FOREACH changes, it should be done in separate patch. Clean dead / commented code, white-space changes, don't remove QObject macro, unless you have a good reason for that. Override is good feature but in separate patch for old code, for new one it's good.
I reverted all the 'cosmetic' change. I just have a problem with the Q_OBJECT macro in KoToolBoxLayout_p.h. If I try to add the Q_OBJECT macro I have then a linking error. I tried to search when this Q_OBJECT were added and I found this commit: R8:703df9162bab8db8a168cca6e4dfec27b8347095
Back-porting patches from Krita are welcome but in accepted condition to other part of the project.
| libs/widgets/KoToolBoxDocker.cpp | ||
|---|---|---|
| 40 | No idea was already like this in the original krita file. | |
Comment Actions
Return QObjects, if you have problems copy linker output here.
| libs/widgets/KoToolBox.cpp | ||
|---|---|---|
| 138 | Revert. | |
| 290 | Mostly foreach do what you expect, revert. | |
| libs/widgets/KoToolBoxDocker.cpp | ||
| 40 | We should know what it is. It used a label instead of KoDockWidgetTitleBar why? Can you revert KoDockWidgetTitleBar, remove this and see how it looks? | |
| libs/widgets/KoToolBoxDocker_p.h | ||
| 42 | Why such a change, i prefer to revert it. | |
| libs/widgets/KoToolBoxLayout_p.h | ||
| 155 | Revert | |
| 163 | Revert | |
| 275 | Remove. | |
| 285–295 | Commented code is no go. | |
Comment Actions
make clean solved the issue
| libs/widgets/KoToolBoxDocker.cpp | ||
|---|---|---|
| 40 | I found out that this QLabel is, it's the small draggable zone at the top of the docker. Actual it's still a bit buggy in krita and karbon  Edit: It's not a bug, it's a feature :D If I try to center the label, it's look a lot less good looking then not zooming on it. I added a small comment for future reference. :) | |
Comment Actions
About the config for the KoKineticScroller. Do you perhaps know where to find the .kcfg file from calligra?
Comment Actions
Config, if it's not set explicitly, <appname> + "rc" https://api.kde.org/frameworks/kconfig/html/kconfig_8cpp_source.html#l00607
Comment Actions
Can you test my suggestions, it's looks good to me.
| libs/widgets/KoToolBoxDocker.cpp | ||
|---|---|---|
| 40 | Please remove label and return KoDockWidgetTitleBar it looks better. | |
| libs/widgets/KoToolBoxLayout_p.h | ||
| 221–228 | Try this code const QSize minSize = minimumSize();
if (!minSize.isValid()) {
return minSize;
}
if (m_orientation == Qt::Vertical) {
return QSize(minSize.width(), minSize.height() * 2 + spacing());
} else {
return QSize(minSize.height() * 2 + spacing(), minSize.width());
} | |
| 270 | We don't need rect so doLayout(rect.size(), true); | |
| 281–282 | return doLayout(QSize(width, 0), false); | |
| 296–297 | return doLayout(QSize(0, height), false); | |
| 310 | Rename notDryRun to applyGeometry | |
| 332–333 | I prefer to be 2 distinct loops if (!applyGeometry) {
foreach (QWidgetItem *wi, m_sections) {
Section *section = static_cast<Section*> (wi->widget());
const int buttonCount = section->visibleButtonCount();
if (buttonCount == 0) {
continue;
}
// rows needed for the buttons (calculation gets the ceiling value of the plain div)
const int neededRowCount = ((buttonCount - 1) / maxColumns) + 1;
if (firstSection) {
firstSection = false;
} else {
// start on a new row, set separator
y += iconHeight + spacing();
}
// advance by all but the last used row
y += (neededRowCount - 1) * iconHeight;
}
return y + iconHeight;
}and other one without notDryRun. | |
| libs/widgets/KoToolBoxLayout_p.h | ||
|---|---|---|
| 310 | Also rename rect to size and QRect to QSize. | |
Comment Actions
Yes work also for me :)
About the title bar for the dock, I'm not a fan of KoDockWidgetTitleBar for the tool bar. Because most of the time the tool bar is too slim to have a draggable zone and you can't drag it like the other toolbar. So you need to click on the button and then drag it.
| libs/widgets/KoToolBoxDocker.cpp | ||
|---|---|---|
| 40 |  | |
Comment Actions
Can you add screenshots of horizontal and vertical version, add it to test, just to have how it looks. I see some gestures that i cannot test, did you can, touchscreen, tablet or something that this code works :)
| libs/widgets/KoToolBoxDocker.cpp | ||
|---|---|---|
| 40 | Feel free to make patch for that, making a Label will not solve the problem. So for now return dock version. | |
| libs/widgets/KoToolBoxLayout_p.h | ||
| 363 | Can try section->hide(), this code is really strange to me. | |
| 369–381 | Remove comment, we not cache anything. | |
Comment Actions
I did some testing with my drawing tablet at home and it's working. I also have a touchscreen on my laptop but I was never able to get it working with qt apps.
| libs/widgets/KoToolBoxLayout_p.h | ||
|---|---|---|
| 221–228 | Changed to just one row/column per default, because Karbon doesn't have for the moment a lot of tools :) | |
Comment Actions
Please, remove all white space changes before commit. It will be better if you test the patch some time without crash or other downsides. If you notice something wrong write here. If all is good you can commit in few days. Thank you for patch, other patches are welcome.
| libs/widgets/KoToolBoxLayout_p.h | ||
|---|---|---|
| 264 | Remove all whitespace change, it doesn't do nothing. | |
Comment Actions
Sorry, I just landed the commit without reading entirely your message, I just saw the comment about fixing the white space problem and not the one about waiting a few days. If I see a problem in the next days I will open a new revision.
That do you recommend as a next task in calligra/karbon that don't need to much knowledge about calligra internals?
Comment Actions
Please contact @bcooksley to remote all white space changes from you commit. Please be more precise when you commit, you introduce even more white spaces rather than to remove them.
That do you recommend as a next task in calligra/karbon that don't need to much knowledge about calligra internals?
If you want we should export markers on svg 1.1, it's not easy, only if you have time and interest on.
Comment Actions
Force pushes are generally reserved for pretty serious issues, couldn't the whitespace be corrected with a follow up commit to remove it?
Comment Actions
I really prefer to not have a whitespace commit, forcing changes commit hash but that's last in master, is there any other downsides ?
Comment Actions
Anyone who has already pulled in the latest changes will need to forcibly reset their local working tree after the force push is completed.
Should they have staged work, this would need to be rebased, which can be a non-trivial process and one many developers will not be familiar with.
For those inexperienced with Git, forcibly resetting their local working copy is often not possible and the only way for them to get back to a usable state is for them to completely erase and re-clone the repository.
In addition, should they have staged changes, the history of those changes could be lost (unless extreme care were taken) and in some cases people have accidentally erased work they've already completed.
For these reasons Force Pushes are generally reserved for situations where no other option is available to correct the issue.