I made a patch yesterday that changes all openConfig() calls to openConfig("krita5rc") -- but then ran into trouble because we both have a krita(5)rc in the Qt resources system and installed to disk, and sometime the embedded one is used, sometimes the installed one, until there is a local one in the user's config folder.

I'm running a potential refactor here and I'm seeing many instances of setting preferences against read-only instances of KisConfig. For instance,

https://invent.kde.org/graphics/krita/blob/master/libs/ui/dialogs/kis_dlg_adjustment_layer.cc#L81

https://invent.kde.org/graphics/krita/blob/master/libs/ui/dialogs/kis_dlg_preferences.cc#L1171

Apart from the sheer inconsistency of setters marked as const, do you know if there's an underlying meaning to this? Since as soon as cfg2 is freed those changes are lost.

Further notes: QSettings does not support temporary/namespaced changes; the handling of synchronization, unlike KConfig, is completely private to Qt.

I was also irritated by the use of "read only" KisConfig instances to manipulate settings, but to me it looks like that is a misleading parameter name. It often gets used as "I don't want you to auto-synchronize the config on destruction, because it will be done later anyway"...

It obviously doesn't prevent you from modifying config settings.
And the changes to that "cfg2" you mentioned are not lost I think, as I understand it, KSharedConfig instances work on the same shared data, i.e. multiple KSharedConfig objects behave like a single KConfig one.
So KisConfig and KisImageConfig just expose different settings of kritarc and can be used in parallel, and hence why one of them is "read only", see above.

But I feel like there should be a "real" read-only mode, and inhibiting auto-sync shall be a separate option.
Designing a nicer configuration class has been going through my head many times, but so far I couldn't come up with something that wouldn't involve some extra tool to turn a nice parameter definition (with type, default, limits) into code.
Trying to do it with some C++14 (or later) variadic template stunts would probably end up in the most unreadable code (heck I have issues following even simple uses, let alone something like Verdigris that implements the Qt MOC functionality as C++ headers) that inflates compile times a ton...

Btw. having arbitrary code change QCoreApplication properties silently sounds scary, I'm not sure what Qt devs thought when making that possible by retrieving the application instance at any place. Maybe we should listen to property-change signals and throw at least a fat warning in your face...

Actually, that's just a bug I introduced accidentally when doing the previous refactoring. Since this is gui code, it should be false...

Further comments after sleeping over it.

QSettings: completely removes synchronization off our shoulders; however, we completely lose control over it, so no temporary or read-only parameters except via const.

K*Config: we retain complete control over sync, since each instance is independent; however, it relies on a mostly undocumented KConfigWatcher to signal resync, and said class depends on DBus. We'd need to borrow QSettings code to implement the signaling ourselves on Windows and Mac.

re files: I see at least three files where we store preferences. It'd be nice to consolidate them:

• karboncalligraphyrc
• kritashortcutsrc
• kritarc

With QSettings:

• kritadisplayrc
• klanguageoverridesrc
• kritasketchpanelsrc

The QSettings files are used in main.cc before the i18n system is initialized. Using kconfig for that would break that.

Hi, @lsegovia!

I tried to refactor KisConfig in 2018, but stopped because it appeared to be too much of work. I tried to solve the following issues, perhaps you can address them in your implementation as well:

1. Changing KisConfig or KisImageConfig requires almost full recompilation of Krita. I wanted to refactor KisConfig into a namespace, so each class could define its own class in this namespace in a local file to define "rules" for config loading/saving. We have a pattern like that in KisDomUtils. QDebug is also a good example. Right now people tend to use KConfigGroup-based ad-hoc code to avoid changing global KisConfig because of that.

2. The read-only parameter of KisConfig is not verified by the compiler. That is the biggest issue of the current implementation.

3. [stretch goal] It would be nice to be able to add versioning to the config. That is, the "local" class describing the rules for the config option would know how to convert the specific option from a previous version to the current one (or just reset it if it sees that the option is from the future). It would solve really a lot of bugs we had in the past (including crashes). Right now we write the checks and conversions in an ad-hoc way, which is (most of the time) rather buggy.

Though I'm not sure what are your plans and whether how that can fit into them.

👋

In T14815#262375, @dkazakov wrote:

1. Changing KisConfig or KisImageConfig requires almost full recompilation of Krita. I wanted to refactor KisConfig into a namespace, so each class could define its own class in this namespace in a local file to define "rules" for config loading/saving. We have a pattern like that in KisDomUtils. QDebug is also a good example. Right now people tend to use KConfigGroup-based ad-hoc code to avoid changing global KisConfig because of that.

I've noticed it. I don't think using a namespaced definition would be OK, because we'd need to leak the implementation details (QDomDocument, QSettings instance) to let the accessors get the values.

2. The read-only parameter of KisConfig is not verified by the compiler. That is the biggest issue of the current implementation.

Currently, I have a KisConfigBase that serves as a shim to access the actual store behind it. I've ensured it's properly consted and, also, I've added two static methods to have disposable instances where appropriate. This has also forced me to go over the setters of KisConfig and change them.

(Now that you say it, perhaps I could go over them a second time and extract non-kritaui preferences into contained subclasses where relevant?)

3. [stretch goal] It would be nice to be able to add versioning to the config. That is, the "local" class describing the rules for the config option would know how to convert the specific option from a previous version to the current one (or just reset it if it sees that the option is from the future). It would solve really a lot of bugs we had in the past (including crashes). Right now we write the checks and conversions in an ad-hoc way, which is (most of the time) rather buggy.

Hmmm, that one will be tricky to do. I'll tackle it after the ones above.

Note: current tree is at work/amyspark/kisconfig.

HI, @lsegovia!

I don't think using a namespaced definition would be OK, because we'd need to leak the implementation details (QDomDocument, QSettings instance) to let the accessors get the values.

Well, my plan was something like that. In this approach all QSettings would be hidden inside the protected interface of KisConfigPropertyBase

// in kis_config.h
namespace KisConfig {

/**
 * Public common implementation that handles constness, versions, conversions and so on
 */
QVariant get(const KisConfigPropertyBase &property); 
void set(KisConfigPropertyBase &property, const QVariant &value);

struct KisConfigPropertyBase {
    virtual QVariant getImpl() = 0;
    virtual QVariant setImpl() = 0;
    virtual void convertVersion(int from, int to) { Q_UNUSED(from); Q_UNUSED(from}; }

protected:
    /// ... some actual interface for accessing the config values by the implementation
};
}

// in some kritaui file we define the property

namespace KisConfig {
struct ConfigUseTimestampsForBrushSpeed : public KisConfigPropertyBase {
    // actual impelemtation of data fetching and conversion
    QVariant getImpl() override;
    QVariant setImpl() override;
};
}

// in some other file we can use the property with ADL-lookup
const bool useTimestamps = get(KisConfig::ConfigUseTimestampsForBrushSpeed()); 

// NOTE: there are two options how to avoid creation of ConfigUseTimestampsForBrushSpeed():
// 1) Precreate static objects for every property, like boost::none (I intended to use this one)
// 2) Pass the type into get as a template parameter

Quick remark, I think this approach is a no-go for our code. Check usages of entryMap().

The one I wanted to try was an One-Ring-like; first, I'd like to make all configuration accessors depend strictly on KisConfigBase (to remove all impl details), and then I'd like to address any refactorings. (Versioning would be a concern still.)

@dkazakov, what would happen if we went SQlite for the preferences handling? That would turn versioning into a plain SQL migration (which I'm not sure if @rempt has dealt with in the past, for the resources refactor).

I would prefer not to go the sqlite route, for the following reasons:

• text editor editable preferences are preferable
• sqlite cannot handle access from threads
• we'd be writing our own settings framework, when there are two settings frameworks already available to us.

If we'd go for a big refactoring, a wrapper around QSettings or kconfig would be preferable.

Hi, @lsegovia!

what would happen if we went SQlite for the preferences handling?

As @rempt said already, we would prefer to keep settings editable by hand. It makes it easier to recover in case of some failures.