Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use KConfig (this commit will probably fail here) #106

Open
wants to merge 1,177 commits into
base: master
Choose a base branch
from

Conversation

cbcalves
Copy link

@cbcalves cbcalves commented Feb 14, 2021

This build will fail if the process flow does not install KDE framworks.

Change all QSettings to use KConfig

Changed the mainwindow to add missing prefixes, there was no change to use static KConfig from last commit.

Carlos Alves and others added 30 commits January 30, 2021 19:31
This is uneeded, we can set this directly on the constructor.
The element will be sligtly bigger as it's holding the two
values, but the overall code is simpler to reason with and smaller
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Carlos Alves and others added 13 commits February 14, 2021 21:28
It can have an undefined behavior.
It can have an undefined behavior.
It can have an undefined behavior.
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Moved vars to private and created access functions.
Organized public, private and  protected repeating.
This class is inherited by all classes in app/element/ and ic.cpp
@tcanabrava
Copy link
Collaborator

can you rebase this to update the CI? master now should be able to compile.

Carlos Alves added 4 commits February 15, 2021 07:51
Removed all QSettings and changed to KConfig:
CMakeLists
bewaveddolphin
icmanager
mainwindow
recentfilescontroller
simplewaveform
thememanager

Created the config files:
wpanda.kcfg
WPandaSettings.kcfgc

Created auxiliary functions in globalproperties
The WPandaSetting is static now.
@vrmiguel
Copy link
Member

vrmiguel commented Feb 16, 2021

Is there a good reason to use KConfig rather than sticking with QSettings?

On the differences between both, KDE states:

  • KConfig allows different backends.
  • KConfig provides kiosk.

Which aren't features we seem to be in need of.

If KConfig isn't super necessary then we should probably pass on adding another build and runtime dependency

Edit: @tcanabrava has now told me about the additional security KConfig offers

@tcanabrava
Copy link
Collaborator

tcanabrava commented Feb 16, 2021

Just as a complementary explanation, as when I explained to @vrmiguel I was mostly sleeping at 2am.

This is KConfigXt, not KConfig. KConfigXT is a configurator generator that makes impossible for you to use the wrong config, and it also gives the application a coherent way to define configurations. KConfig is the same thing as QSettings with the same possible errors.

The problems that happen with QSettings:

  • The settings are defined in runtime, and because of that it's not possible to know what settings an application has
  • nor even grepping the code for QSettings work, as we could have this kind of code:
void setSettings(const QString& setting, const QVariant value) {
      QSetting s;
      s.setValue(setting, value);
}

It's also extermely dangerous to use string identification for settings:

void load() {
    QSettings s;
    m_someValue = s.value("context").toInt();
}

void save() {
    QSettings s;
    s.setValue("contest", m_someValue);
}

It's easy to spot the typo in contest / context. but it's not easy to spot the typo with 20 other settings laying around.
Those are the issues that KConfigXT solves.

@liomarhora
Copy link
Contributor

Is this program still being ported to KDE?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants