[RFC] General config interface
AbandonedPublic

Authored by loh.tar on Feb 10 2019, 9:21 AM.

Details

Reviewers
cullmann
dhaumann
Summary
  • Remove unneeded bool configIsRunning, add m_ prefix to configSessionNumber
  • Remove unneeded inline keyword

Pro

  • Much easier to add new options/settings
  • Less ever the same boring code

Cons

  • Slower access
  • Presumably more memory consumption

Notes

  • Only three example settings in use, "Word Wrap"/bool, "Word Wrap Column"/int and "Indentation Mode"/string
  • No use of enum as key, but const char *, to keep it simple
  • Use Q_ASSERT to avoid wrong written keys

TODO/NeedHelp

  • See code comments
  • FIX constness of getter functions
  • Apply in a general way to
    • DocumentPrivate::readVariableLine
    • KateCommands::CoreCommands
    • VariableLineEdit::addKateItems
  • Finish/improve documentation

Questions/Hints

  • See code comments
  • Is setting "BackupFlags" really needed?
  • Are all these SomeClass:: get/set functions really needed? e.g: DocumentPrivate::wordWrapAt() seems to be unused; DocumentPrivate::setWordWrapAt(uint col) effective only 4 times used; Instead is often accessed directly by doc->config()->foo(); So perhaps could the way over fooClass()->config() be done in general

Diff Detail

Repository
R39 KTextEditor
Lint
Lint Skipped
Unit
Unit Tests Skipped
loh.tar created this revision.Feb 10 2019, 9:21 AM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptFeb 10 2019, 9:21 AM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Feb 10 2019, 9:21 AM

This approach spreads they key "Word Wrap Column" across many files. You have to know the key and avoid typos. Currently we have this hard-coded and therefore statically checked by the compiler, which is very good.

In grneral, I am not against the string based approach in addition to a statically checked approach. The string based setter then would take a QVariant as value. This way, we would also be close to the KTextEditor::ConfigInterface and could have a KTextEditor::Command 'set', i.e. F7: set word-wrap-column 80.

@mwolff Didn't you once propose a template based approach to this?

I have the feeling there are better solutions that perform just as well.

Other comments?

PS: yes, currently we have to write a lot of boilerplate code, but this is simple, the code is fast, and the functions are compile time checked.

Contrary to my announcement to use an enum as key I chosed the string for maximum simplicity.
Yes, this way you notice a typo only at runtime, but in any case you have to know how some stuff has to be written. An enum key offer not only complile time checks but also to use QVector instead of QHash which may be more perfomand but need some more effort when add new stuff and probably longer longer keys, prefixed by namespace.

My only goal is to get rid of all these tiny functions. But I understand you so, that you don't see a need for that.

First: the current framework for settings suck, I feel guilty.
Second: I think a QVariant based approach would be nicer.

I should think about how to do that best.

Dominik, didn't you have some generic property stuff in your TikzKit?

Hi,

sorry that I didn't look earlier at this ;=)

I think in principle, that goes into the right direction.

For more compile-time safety I would like to have enums.

For the internal design: I would use the QVariant types and I would avoid that explicit isGlobal/local stuff in favor of a parent pointer.

I will hand in an altered variant as an extra diff, then we can converge.

loh.tar abandoned this revision.Mar 4 2019, 8:03 PM

In favor of D19491