generic config interface
ClosedPublic

Authored by cullmann on Mar 3 2019, 11:27 AM.

Details

Summary

instead of member per entry, use key/value with QVariant
instead of manual local/global config, implement this via parent pointer
optional validator via std::function

one example use: setTabWidth

Test Plan

make && make test

Diff Detail

Repository
R39 KTextEditor
Branch
arcpatch-D19491_1
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9674
Build 9692: arc lint + arc unit
cullmann created this revision.Mar 3 2019, 11:27 AM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptMar 3 2019, 11:27 AM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
cullmann requested review of this revision.Mar 3 2019, 11:27 AM

Other approach to generic config.
Takes the ideas of D18894 with some tweaks.
Uses enums for the keys (thought ATM puts that into an int in the interface, one might alter that later with some templatizing of the KateConfig.

cullmann updated this revision to Diff 53063.Mar 3 2019, 1:36 PM

better error checking for ::value
fix coding style

Any input?
We could give this a go "after" the next frameworks release is out.

Currently, one cannot 'unset' a value, but I guess that is ok, since we did not have that before

In general this approach is OK. Some thoughts on this:

  • is it as fast as before, if you start Kate with e.g. 100 documents open?
  • will this allow easy access through the KTextEditor::Config interface? This is a Document and View extension interface.
  • can we allow modification of the config values through a generic KTextEditor::Command? I.e. set <key> <value>? Maybe also reset <key>?
  • Can we make this accessible through the upcoming variable interface, e.g. as ${CurrentDocument:Config:<key>}?

As you can see, I am trying to push this concept a bit further...

Currently, one cannot 'unset' a value, but I guess that is ok, since we did not have that before

In general this approach is OK. Some thoughts on this:

  • is it as fast as before, if you start Kate with e.g. 100 documents open?

:=) No idea, for each setting you now dow a hash lookup.
We could optimize that to use a vector with the enum as index, given we enumerate them from 0...
But I think we can only judge that after one has converted close to all things.

  • will this allow easy access through the KTextEditor::Config interface? This is a Document and View extension interface.

I think we can just add a second key for that to the ConfigEntry specifying its name there

  • can we allow modification of the config values through a generic KTextEditor::Command? I.e. set <key> <value>? Maybe also reset <key>?

We can use the same key there and we have with the defaultValue QVariant even the type of the config entry.
Reset is easy, too, as we just can clear it from the hash (for the local configs), for the global config I don't think it makes sense (we could go to the hard-coded default, thought)

  • Can we make this accessible through the upcoming variable interface, e.g. as ${CurrentDocument:Config:<key>}?

Same as above, either extra key or "CamlCase" the key we use for the command-line/kate: var line

As you can see, I am trying to push this concept a bit further...

:=) Yes, and that is clever.
I think given we now encode each config entry as "data", it should be easy to add all these things.

either extra key or "CamlCase" the key we use for the command-line/kate: var line

I would avoid any kind of extra key. Just one string used for the config file and these document variables. I guess "Tab Width" is simpler to convert to "set-tab-width" as "TabWidth"

I think some extra keys are not avoidable to be compatible with the old stuff.
See e.g.

// BEGIN ConfigInterface stff
QStringList KTextEditor::DocumentPrivate::configKeys() const
{

static const QStringList keys = {
    QLatin1String("backup-on-save-local"),
    QLatin1String("backup-on-save-suffix"),
    QLatin1String("backup-on-save-prefix"),
    QLatin1String("replace-tabs"),
    QLatin1String("indent-pasted-text"),
    QLatin1String("tab-width"),
    QLatin1String("indent-width"),
    QLatin1String("on-the-fly-spellcheck"),
};
return keys;

}

If we have these as extra keys, we can just auto-generate that stringlist (and have all settings exposed for free).
The configValue() + setConfigValue() can then just call directly the setValue()... things, too, after looking up the enum from a hash.

Well, when it is important to be compatible, then may that needed. OTH would it be so terrible to break it?

Perhaps it may more important to keep the doc variables untouched than the config file keys. So I would adjust the config file keys to fit the doc variables. This way you can every kind of setting adjust by such doc variable.

I think breaking the config files is just as bad ;=)

After we have converted all things, we might even support exporting/importing the complete document/view/render global config to from other formats like json using the "nice" keys, as we can just write a small load/store routine for that.

I think such a thing was wanted long ago, too, to transfer the settings somewhere (or store per project) without ini file hassle.

Just one extra key per entry is really not hard, you do that once per setting.

I would not introduce a second one for the CamelCaseStyle variable thought.
If we need that, I would just CamelCase the tab-width style name. That is easily automated in code.

cullmann updated this revision to Diff 54021.Mar 16 2019, 3:51 PM

add command name, too
hide data structure

I would like to get this in now.
The next step would be converting all existing settings to the new style like tabWidth but keeping the accessors as is, to have a minimal large change.
If that then is done, one can start to think about using the generic interface and removing the specific accessors (after adding some convenience functions for bool/int/... values perhaps).
Next step then would be to use the command stuff in the ConfigInterface, this would directly allow to expose all settings without any extra code.

dhaumann accepted this revision.Mar 17 2019, 8:21 AM

Looks good to me.

I see two possible performance issues:

  1. Using std::map. ...but the size() is small.
  2. See comment.

But that needs profiling later.

Please push.

src/utils/kateconfig.cpp
124

Possible performance issue? We'll see later.

This revision is now accepted and ready to land.Mar 17 2019, 8:21 AM

Performance must later be profiled after it is used a lot.
Then we can e.g. still switch to a vector indexed with the enum value, that should make lookup fast.

This revision was automatically updated to reflect the committed changes.

Agreed, do you plan to migrate more config options now?