HACKING
Rama master · réplica de lectura del repositorio original.
Since 1999, people have been hacking on Krita. Everyone brought their
own coding style, their own code conventions, their own likes and
dislikes. Me, (Halla that is), I like indents of four spaces, and
no scope prefixes for variables. However, in the interests of
consistency, these are the rules new code should adhere to:
See also https://community.kde.org/Policies/Frameworks_Coding_Style --
that document is leading.
Qt vs STD vs Boost:
In general, use the Qt classes wherever possible, even if someone tells you
that the STD class is better or whatever. We're dealing with a big project
with lots of developers here and we should keep the code as consistent and
easy to grasp as possible. Since people need to know Qt in the first place,
keep to Qt. Discuss deviations on #krita
C++11 and C++14
Yes, but.
* Prefer normal functions over lambdas, unless really needed (e.g. when replacing
the use of QSignalMapper)
* Try to avoid auto as a shortcut for the type name, except when used
in range-based for-loops and iterators. Using auto as a replacement
for a templated code (or inside templated code) is perfectly fine.
* Generic lambdas. Use generic lambdas only when you want to solve a
problem that would otherwise require templates, not just as a shortcut for
the argument typenames.
* auto as a deduced return type of functions that would otherwise require
templates is perfectly fine
* In some cases it might be okay to use auto as a shortcut for the typename,
e.g. when using very long types returned by boost. But better discuss that
on #krita.
* Avoid the new sig/slot connection syntax _unless_ you are porting all of
Krita to the new syntax. Sure, it has some advantages, but having two different
ways of doing the same thing is begging for trouble and comprehension problems.
* For now, keep using Q_FOREACH, we're using it all over the place, and it has
different constness semantics over the standard range-based for-loop.
* Use nullptr in new code. When changing existing code, make sure that 1) the whole
.cpp/.h files pair uses the same style, 2) the change of the style outweigh the
loss of the git-blame history (that is, your changes do already change the file
significantly)
* Before using other new features, discuss on #krita so we can expand this list.
Indentation
With four spaces. Use the default kdelibs indentation
(http://techbase.kde.org/Policies/Kdelibs_Coding_Style)
Indentation of Lambdas
For indentation of lambdas refer to the `LambdaBodyIndentation: Signature` method used
by clang-format, i.e. "align lambda body relative to the lambda signature"
someMethod(
[](SomeReallyLongLambdaSignatureArgument foo) {
return;
});
You can use automated "Format Selection" actions of QtCreator and VSCode to format
lambdas. They will user the necessary settings from .clang-format file present in
the Krita's source tree.
Includes
Avoid as much as possible #includes in header files; use forward declarations
of classes.
Initializers
Avoid as much as possible initializers in the body of the constructor. Use
initializer lists instead. Write the initializers as follows
Class(A a, B b)
: Subclass(a)
, m_b(b)
{
}
Note the location of the colon and comma.
It is also preferred to use {}-initialization for class members and
global namespace scope variables, e.g.
class Foo
{
int m_value {0};
};
namespace Bar
{
int SomeGlobalValue {0};
};
Since Krita has a long history of usage =-initialization everywhere, it is **not**
recommended to use {}-initialization in function bodies, especially, when it results
in a mix of styles in the same file. You can still use that as an exception to solve
some specific problems like the most vexing parse.
Scope prefixes
Use only m_ for class-level variables. No other scope prefixes; no g_, l_,
no 'p' for pointer variables.
Shared pointers
Use shared pointers wherever possible. Prefer Qt's shared pointer classes
to our home-grown shared pointer classes.
QScopedPointer is being deprecated by Qt
We use QScopedPointer **only** for a d-ptr pattern in simple classes not
supporting move-semantics. For all other usages, use std::unique_ptr.
Getter/setter
Krita doesn't use Qt's properties -- yet. If you want to introduce use of
properties, convert any and all classes in Krita before committing.
Getter/setters are named 'x() for getters and setX(int x) for setters. If you
come across violations of this rule, change the code.
Class naming
If you use a well-known design pattern, name the class according to the design
pattern. All files should start with 'Kis', all classes with the 'Kis' prefix.
This filename should be the same as the classname: KisNewClass.h, KisNewClass.
Filenames in plugins do not start with Kis; only in libraries. Do not make new
classes that start with Ko.
Function naming
Functions should be named in camelBackedFashion, to conform to Qt's standards.
If you encounter functions in c_style_like_this, feel free to rename. Also:
verbNoun -- i.e., rotateLayer, not layer_rotate. The latter is a true c-ism,
introduced by a language that needs to prefix the 'class' name to every function
in order to have something that's not quite OO.
Variable/Parameter names
Variable/parameter names start with a lower case letter. A name composed of different
words is done in camelBackedStyle.
Try to avoid abbreviations except for the most common cases, like cs for KoColorSpace.
It's okay for variable names to be long and explicit and derived from the type name.
Designer
Krita has started to use designer. All dialogs and all widgets that have a layout
manager must be done in designer. Do not add code or signal/slot connections
in designer.
Enums
All enums should be prefixed with 'enum'.
Namespaces
Currently, we only use anonymous namespaces for things like undo
commands. For the rest, some classes have a 'Kis' prefix, others don't. This should
be made consistent, and we might want to use namespaces to keep all of Krita
inside.
Files and classes
It's preferred (and strongly preferred) to have only one class per .h/.cpp file.
(Which is logical, because otherwise you won't be able to keep to the naming scheme.)
Spaces
Keep the source airy and open. In particular, there should be empty lines between function
declarations and definitions.
Slots and signals
Prefix slots with slot and signals with sig: slotUpdateSelection, sigSelectionUpdated.
Boolean operators
Use the standard !, !=, ==, && etc style, not the "not", "and" etc. style. Keep krita code
using one, easily recognizable, C++ style.
Conversion of QScopedPointer and QSharedPointer to booleans
With the update to Qt6 we cannot longer use implicit conversions of smart pointers
to booleans. Instead we should use convert the result to boolean explicitly:
Qt5 way:
QSharedPointer<KisPaintOpPreset> preset = <...>;
const bool hasPreset = preset; // does not work anymore!
Qt6 way:
QSharedPointer<KisPaintOpPreset> preset = <...>;
const bool hasPreset = bool(preset); // convert to bool explicitly!
Please try to avoid constructions involving `!preset.isNull()`, they
are hard to read.
Static initializers
Sometimes we need to declare a static object that performs some actions on Krita
loading, e.g. to register Qt's metatype for a Krita type. We have a special macro
for that:
KIS_DECLARE_STATIC_INITIALIZER {
qRegisterMetaType<KoResourceSP>("KoResourceSP");
}
What the macro does is basically defining a static variable that is initialized
on loading of the library:
struct KoResourceSPStaticInitializer {
KoResourceSPStaticInitializer() {
qRegisterMetaType<KoResourceSP>("KoResourceSP");
}
};
static KoResourceSPStaticInitializer __initializer1;
This macro is usually added to the same .cpp file as the class itself.
WARNING: do not use this pattern in **plugins**. All Krita's plugins are
actually static libraries that are pulled into dynamic .so libraries at the
linking stage. It means that all "unused" objects will never be pulled
into the final plugin. Instead, do all the initialization in the "plugin"
class (the one that is passed to registerPlugin<>() in
K_PLUGIN_FACTORY_WITH_JSON).
The problem can be worked around with `$<LINK_LIBRARY:WHOLE_ARCHIVE,...>`
CMake feature, but it is available only since CMake 3.24, so for now
just avoid using initializers in plugins.
The usage of timers, compressors and QTimer::singleShot()
We should be carefult about allocation of timers in Krita. The number of precision
timers is limited on some platforms (e.g. Windows). When Qt runs out of precision
timers it silently degrades all newly created timers into non-precision ones. That
can affect timers that are used for the canvas framerate stabilization and cause
tearing and FPS drops.
Rules of thumb:
1) When using QTimer::singleShot(), either use **null** timeout to just push the
event through the queue, or pass Qt::CoarseTimer as the second argument:
QTimer::singleShot(0, ...);
or
QTimer::singleShot(<some non-zero value>, Qt::CoarseTimer, ...);
If you really-really-really need a precise single-shot event, then pass
Qt::PreciseTimer explicitly.
The problem is that QTimer::singleShot() creates a precise timer by default,
which can consume all the available precise timers in the system for the app.
2) When creating timers or using KisSignalCompressor, set timeout to values
strictly higher than 20 ms (unless you really know what you are doing). Value
`25 ms` is a good default to compress updates for the lightweight widgets,
and value `100 ms` is a good default to compress updates for heavy widgets.
The problem is that Qt internally converts all the timers into precise
ones, if their timeout is less or equal 20. Which can also consume all
the available precise timers.
Class template argument deduction (CTAD) (since C++17)
C++17 allows automatical deduction of class template arguments for the class
constructors. I.e. you can write
QList list({QPointF(), QPointF(), QPointF()});
and template arguments for the QList template will be deduced automatically.
You can use this feature in new code, given the style of the whole file is
consistent.
WARNING for Qt6 and QVector:
Do **NOT** use CTAD for QVector until we switch to C++20! In Qt6 QVector is
declared as an alias for QList, and CTAD for aliases is a C++20 feature!
The code **will** compile with GCC on Linux even with `--std=c++17`, but
will fail to compile with Clang on Windows, Android and MacOS.
The usage of custom patches in Qt
In Krita we use a lot of custom patches on Qt. An important part of this works
is to make sure Krita still works with unpatches Qt, so that Linux distributions
could package it.
To make Qt patches optional and maintainable, please use the follwoing guideline:
1) Detect Qt patches in main CMakeLists.txt file using `check_cxx_source_compiles`
or `check_cxx_symbol_exists`:
# modify includes and libraries if necessary
set(CMAKE_REQUIRED_INCLUDES ${Qt6Gui_INCLUDE_DIRS} ${Qt6Gui_PRIVATE_INCLUDE_DIRS})
set(CMAKE_REQUIRED_LIBRARIES ${Qt6Gui_LIBRARIES})
CHECK_CXX_SOURCE_COMPILES("
#include <QtGui/rhi/qrhi.h>
int main(int, char *[]) {
bool (QRhi::*x)() const;
x = &QRhi::isLastFrameCompletedOnGPU;
Q_UNUSED(x);
return 0;
}
"
KRITA_QT_HAS_UPDATE_COMPRESSION_PATCH
)
# reset modified environment
unset(CMAKE_REQUIRED_INCLUDES)
unset(CMAKE_REQUIRED_LIBRARIES)
2) [optional] If the patch is important enough, or if the developer might want
to disable it for some reason, add it to the list of features:
option(KRITA_QT_HAS_UPDATE_COMPRESSION_PATCH
"Qt has a custom patch for limiting FPS to the screen framerate"
ON)
add_feature_info("Qt: Compressed updates patch"
KRITA_QT_HAS_UPDATE_COMPRESSION_PATCH
"Krita will compress the screen updates to the maximum FPS of available displays")
3) Add the definition for the detection variable into `config-qt-patches-present.h.cmake`
#cmakedefine01 KRITA_QT_HAS_UPDATE_COMPRESSION_PATCH
4) At the point of usage just include the auto-generated include and check for the
feature to exist
#include <config-qt-patches-present.h>
#if KRITA_QT_HAS_UPDATE_COMPRESSION_PATCH
// do your work
#endif /* KRITA_QT_HAS_UPDATE_COMPRESSION_PATCH */
Notes:
1) Try to avoid inclusion of `config-qt-patches-present.h` into header files. Include
that into .cpp only (just to avid too much compile dependencies).
2) If you don't add your patch into the list of features, its status can still
be tracked on CI via `CMakeConfigureLog.yaml` file, which is available as an
artifact of the build.
On menu action names: "Group layers" vs "Group layer(s)"
tldr; use simple plural form "Group layers" instead of "Group layer(s)"
when you don't know how many items the action is going to process
Sometimes, the meaning of a user-facing action depends on the actual
number of items the user selected, e.g. the number of selected layers
or vector shapes. So we cannot prepare a precise wording for this
action upfront. In this case, use the following rules to name the
action:
* if an action is defined in an `.action` file, i.e. present
`Configure Shortcuts` dialog, just use a simple plural form
"Group Layers".
Do not use the form with "(s)" in the end (e.g. "Group layer(s)"),
because it is impossible to translate that correctly into
non-English languages.
Also try to avoid adjusting the name of such action at runtime
using `i18ncp` call, because it will also change the name of the
action in `Configure Shortcuts` dialog.
* if the action is created at runtime, and is not present
in `Configure Shortcuts` dialog, just use `i18np` to handle
plural forms correctly:
i18ncp("@action:inmenu", "Group layer", "Group layers")
Case insensitive string comparison
Instead of using `str.toLower() == "abc"` prefer using
`str.compare("abc", Qt::CaseInsensitive) == 0`, especially in hot
places
The `QString::toLower()` call is expensive on Winodws, since it allocates
memory on heap. It is more efficient to perform the comparison in place.
With Krita now supporting Python scripting, we need guidelines for these as well.
These guidelines are preliminary and may be further refined in the future.
To keep it simple, we have chosen to follow the style guide suggested by Python: PEP8.
All rules should be followed, except the max limit of 79 characters per line. As this
can reduce readability in some cases, this rule is optional.
The full PEP8 specification is available here: https://www.python.org/dev/peps/pep-0008/
To check compliance you can run pep8.py against the code.
You can also use autopep8.py to automatically fix detected compliance issues.
pep8.py can be downloaded via Python's package manager (pip) [https://pypi.python.org/pypi/pep8],
or your distribution's package manager.
autopep8.py can also be downloaded via Python's package manager [https://pypi.python.org/pypi/autopep8],
or your distribution's package manager.
Both of these scripts come bundled with the PyDev plugin, which is available for Eclipse and other IDEs.
The PyDev integration can be configured to visually highlight portions of the code which is not in compliance,
as well as run autopep8 via shortcuts.
pep8.py and autopep8.py can suppress select rules via the "--ignore" command line argument.
To ignore the 79 characters per line rule, pep8.py can be called like this:
pep8.py --ignore=E501
You can read more about the error codes and what they mean here:
http://pep8.readthedocs.io/en/release-1.7.x/intro.html#error-codes
QML Notes:
While the hacking file is not a good place to discuss human interface design, there are some technical
oddities with QML to take into account:
- Height, Implicit Height and Content Height can get very confusing. In theory, implicit height is only used
when height isn't set, but this is complicated by delegates never using the height property, and only using
implicit height. Furthermore, contentHeight, and contentItem.implicitHeight are only relevant when the item
has a contentItem, like with AbstractButtons and PopUps, but commonly, the implicit height of these
is already set to the contentHeight, so doesn't need to be set (and can in fact lead to slowdowns).
Conversely, if no implicit height is set on the content item, the content item will take the height
and width from the parent button or popup.
- Similarly, ListViews are not supposed to instantiate delegates outside of their views, but if a ListViews
is instantiated with their height set to contentHeight, then, even as the ListView resizes itself later, it
will instantiate a delegate for every single item it could lay out upon the ListView itself be instantiated.
It's better to set the default height of the ListView to something bounded, like the fontSize * 3, and then
ensure it is resized later.
- Comboboxes expect their delegates to be a ItemDelegate or AbstractButton. The Qt Docs imply this is because
those call activated(), but Combobox.highlightedIndex being set on hover also only works together ItemDelegate.