Differential D14777

Refactor KisImageConfig to disallow writting settings in read only mode
Changes PlannedPublic
Actions

Authored by dkazakov on Aug 13 2018, 8:21 AM.

Details

Reviewers

rempt

Group Reviewers

Krita

Summary

Now we have two versions of KisImageConfig:

KisImageConfig -- read only type of the config, in which you can request only getters. Accessing any setter method will throw a compile-time error (due to SFINAE)

KisImageConfigMutable -- fully featured version of the config object that allows setters and that dumps all the settings to the filesystem on destruction.

Implementation details:

The thing is implemeted using templates and SFINAE. The
original KConfig object is wrapped into special wrappers:
KisKConfigWrapper and KisKConfigMutableWrapper. The former
one forwards all readEntry calls to KConfig, the latter
forwards everything, including writeEntry and deleteEntry.

KisImageConfig is implemented as a template and has two explicit
specializations for KisKConfigWrapper and KisKConfigMutableWrapper.
Setter methods of the class access m_config.writeEntry, so they
become disabled for a read-only specialization, based on
KisKConfigWrapper, which doesn't have these methods.

Test Plan

Review the code and say if you like it or now. You can also try to
build it. If it builds fine and Krita starts without any crashes, then
everything is fine.

Diff Detail

Repository

R37 Krita

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 1754
Build 1772: arc lint + arc unit

dkazakov created this revision.Aug 13 2018, 8:21 AM

Restricted Application added a project: Krita. · View Herald TranscriptAug 13 2018, 8:21 AM

dkazakov requested review of this revision.Aug 13 2018, 8:21 AM

Harbormaster completed remote builds in B1754: Diff 39557.Aug 13 2018, 8:21 AM

Maybe it is an idea to add to the description what kind of things this solves? Like, when do we use read-only mode for this object?

In D14777#310541, @woltherav wrote:

Maybe it is an idea to add to the description what kind of things this solves? Like, when do we use read-only mode for this object?

Good idea. I'll add a description. Basically, the patch solves the misunderstanding issues, like happened in this patch with KisConfig :)

I don't really like the macros, but I guess there's nothing else we could do. For the rest, the naming needs a bit of attention, I think, that's noted in-line.

libs/global/KisCpp14Quirks.h
26	I guess this needs some documentation.
libs/image/KisKConfigWrapper.h
49	I'm not sure I wouldn't prefer KisConfigReadOnly and KisConfigReadWrite instead of KisImageConfig and KisImageConfigMutable.
libs/image/kis_image_config.h
32	Hm... There's already Q_DECL_, should we namespace our DECL_ with KIS_ ? Also, DECL_PROPERTY sounds a bit like Q_PROPERTY, but that's different. KIS_CONFIG_GETTER and KIS_CONFIG_GETTER_SETTER might be clearer?

This revision is now accepted and ready to land.Aug 17 2018, 9:53 AM

The think I dislike about the patch is the fact that all this weird code is inlined. Given that we include kis_config.h and kis_image_config.h almost everywhere, it might really affect compilation time. And not even talking about the general understanding by people. I will think about some other approach without templates and inlined code...

// all the functions of this class are explicitly instantiated in a special .cpp file
template <typename T>
struct Property {
    Property(const std::string &key, T defaultValue);
    virtual T get() const;
    virtual void set(T value) const;
    virtual T defaultValue() const;
    std::string key() const;

private:
    std::string m_key;
    T m_defaultValue;
};

#define MY_PROP(_name, _type, _default) \
    static const Property<_type> _name(#_name, _default)

namespace KisConfig {
MY_PROP(someNiceValue, int, 256);
}

Revision Contents
Changeset List

		Path
M		benchmarks/KisAnimationRenderingBenchmark.cpp (2 lines)
M		benchmarks/kis_low_memory_benchmark.cpp (2 lines)
A	M	libs/global/KisCpp14Quirks.h (34 lines)
M		libs/image/CMakeLists.txt (1 line)
A	M	libs/image/KisKConfigWrapper.h (66 lines)
A	M	libs/image/KisKConfigWrapper.cpp (40 lines)
M		libs/image/kis_image.cc (4 lines)
M		libs/image/kis_image_config.h (290 lines)
M		libs/image/kis_image_config.cpp (463 lines)
M		libs/image/kis_keyframe.cpp (2 lines)
M		libs/image/kis_memory_statistics_server.cpp (2 lines)
M		libs/image/kis_onion_skin_compositor.cpp (2 lines)
M		libs/image/kis_selection_mask.cpp (2 lines)
M		libs/image/kis_simple_update_queue.cpp (2 lines)
M		libs/image/kis_transaction_data.cpp (2 lines)
M		libs/image/kis_transform_mask.cpp (2 lines)
M		libs/image/kis_update_scheduler.cpp (4 lines)
M		libs/image/kis_update_time_monitor.cpp (2 lines)
M		libs/image/krita_utils.cpp (2 lines)
M		libs/image/lazybrush/kis_colorize_stroke_strategy.cpp (2 lines)
M		libs/image/tests/kis_onion_skin_compositor_test.cpp (4 lines)
M		libs/image/tiles3/kis_tile_data_pooler.cc (4 lines)
M		libs/image/tiles3/swap/kis_swapped_data_store.cpp (2 lines)
M		libs/image/tiles3/swap/kis_tile_data_swapper_p.h (2 lines)
M		libs/image/tiles3/tests/kis_low_memory_tests.cpp (2 lines)
M		libs/image/tiles3/tests/kis_store_limits_test.cpp (2 lines)
M		libs/image/tiles3/tests/kis_swapped_data_store_test.cpp (4 lines)
M		libs/image/tiles3/tests/kis_tile_data_store_test.cpp (2 lines)
M		libs/ui/canvas/kis_animation_player.cpp (2 lines)
M		libs/ui/canvas/kis_canvas2.cpp (8 lines)
M		libs/ui/canvas/kis_image_pyramid.cpp (2 lines)
M		libs/ui/canvas/kis_prescaled_projection.cpp (2 lines)
M		libs/ui/dialogs/KisAsyncAnimationRenderDialogBase.cpp (2 lines)
M		libs/ui/dialogs/kis_dlg_image_properties.cc (2 lines)
M		libs/ui/dialogs/kis_dlg_preferences.cc (18 lines)
M		libs/ui/kis_animation_frame_cache.cpp (2 lines)
M		libs/ui/kis_selection_decoration.cc (2 lines)
M		plugins/dockers/animation/animation_docker.cpp (6 lines)
M		plugins/dockers/animation/onion_skins_docker.cpp (8 lines)
M		plugins/dockers/animation/timeline_frames_view.cpp (4 lines)
M		plugins/impex/libkra/kis_kra_loader.cpp (4 lines)
M		plugins/paintops/defaultpaintops/brush/kis_brushop.cpp (2 lines)
M		plugins/tools/tool_transform2/strokes/transform_stroke_strategy.cpp (2 lines)

Commit	Tree	Parents	Author	Summary	Date
4c1437b767ae	d82a3e9106ac	ca1b82fa5405	Dmitry Kazakov	Refactor KisImageConfig to disallow writting settings in read only mode (Show More…)	Aug 13 2018, 8:19 AM

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	39557	ca1b82f		Aug 13 2018, 8:21 AM	★	★

Refactor KisImageConfig to disallow writting settings in read only modeChanges PlannedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 39557

benchmarks/KisAnimationRenderingBenchmark.cpp

benchmarks/kis_low_memory_benchmark.cpp

libs/global/KisCpp14Quirks.h

libs/image/CMakeLists.txt

libs/image/KisKConfigWrapper.h

libs/image/KisKConfigWrapper.cpp

libs/image/kis_image.cc

libs/image/kis_image_config.h

libs/image/kis_image_config.cpp

libs/image/kis_keyframe.cpp

libs/image/kis_memory_statistics_server.cpp

libs/image/kis_onion_skin_compositor.cpp

libs/image/kis_selection_mask.cpp

libs/image/kis_simple_update_queue.cpp

libs/image/kis_transaction_data.cpp

libs/image/kis_transform_mask.cpp

libs/image/kis_update_scheduler.cpp

libs/image/kis_update_time_monitor.cpp

libs/image/krita_utils.cpp

libs/image/lazybrush/kis_colorize_stroke_strategy.cpp

libs/image/tests/kis_onion_skin_compositor_test.cpp

libs/image/tiles3/kis_tile_data_pooler.cc

libs/image/tiles3/swap/kis_swapped_data_store.cpp

libs/image/tiles3/swap/kis_tile_data_swapper_p.h

libs/image/tiles3/tests/kis_low_memory_tests.cpp

libs/image/tiles3/tests/kis_store_limits_test.cpp

libs/image/tiles3/tests/kis_swapped_data_store_test.cpp

libs/image/tiles3/tests/kis_tile_data_store_test.cpp

libs/ui/canvas/kis_animation_player.cpp

libs/ui/canvas/kis_canvas2.cpp

libs/ui/canvas/kis_image_pyramid.cpp

libs/ui/canvas/kis_prescaled_projection.cpp

libs/ui/dialogs/KisAsyncAnimationRenderDialogBase.cpp

libs/ui/dialogs/kis_dlg_image_properties.cc

libs/ui/dialogs/kis_dlg_preferences.cc

libs/ui/kis_animation_frame_cache.cpp

libs/ui/kis_selection_decoration.cc

plugins/dockers/animation/animation_docker.cpp

plugins/dockers/animation/onion_skins_docker.cpp

plugins/dockers/animation/timeline_frames_view.cpp

plugins/impex/libkra/kis_kra_loader.cpp

plugins/paintops/defaultpaintops/brush/kis_brushop.cpp

plugins/tools/tool_transform2/strokes/transform_stroke_strategy.cpp

Refactor KisImageConfig to disallow writting settings in read only mode
Changes PlannedPublic
Actions

Revision Contents
Changeset List