Port from GConf to GSettings
ClosedPublic

Authored by nicolasfella on Jul 15 2018, 10:28 PM.

Details

Summary

CCBUG: 386665

As discussed in bug 386665 GConf is deprecated and needs to be replaced by GSettings to keep features enabled.

Question: Do we need GConf as a fallback for PA versions without module-gsettings?

Test Plan

Checkboxes in Advanced tab are enabled again.
Changed settings are shown in dconf-editor and vice versa.
Combine output checkbox shows/hides combined sink in applet

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nicolasfella created this revision.Jul 15 2018, 10:28 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 15 2018, 10:28 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
nicolasfella requested review of this revision.Jul 15 2018, 10:28 PM
drosca requested changes to this revision.EditedJul 16 2018, 5:56 AM

It still needs to keep GConf code for systems without gsettings pulseaudio module (eg. pulseaudio < 12.0).

It should be configurable which one to build, with gsettings being default if found.

Also make the Qt keywords changes in separate commit or include gsettings headers as last with #undef signal.

This revision now requires changes to proceed.Jul 16 2018, 5:56 AM

I would prefer those keyword changes to be done separately

nicolasfella added a comment.EditedJul 16 2018, 2:24 PM

Keyword changes were made in D14157

  • Support both GConf and GSettings
  • Don't require GIO
  • Fix copyright
nicolasfella added a comment.EditedJul 16 2018, 3:59 PM

It now defaults to GSettings if found and one can pass -DUSE_GCONF=TRUE to build it using GConf

drosca requested changes to this revision.Jul 16 2018, 7:40 PM
drosca added inline comments.
CMakeLists.txt
52

Those two options (gconf, gsettings) should be mutually exclusive.

src/kcm/package/contents/ui/Advanced.qml
82

This should still check for "module-gconf" or "module-gsettings" being loaded in pulseaudio.

The way you changed it is that it will just completely hide those options if appropriate module is not loaded in pulseaudio without any indication to user what is wrong.

This revision now requires changes to proceed.Jul 16 2018, 7:40 PM
  • Fail if user sets both USE_GCONF and USE_GSETTINGS
  • Restore warning when module is unloaded
  • Remove unneeded class declaration
drosca added inline comments.Jul 17 2018, 6:45 AM
CMakeLists.txt
44

This should probably use cache variables:

set(USE_GCONF FALSE CACHE STRING "Build with GConf")
set(USE_GSETTINGS TRUE CACHE STRING "Build with GSettings")
src/gsettingsitem.cpp
23

not needed

27

local includes should be the first in file

41

nullptr

54

newline

64

indentation

74

braces

83

newline

92

braces

src/gsettingsitem.h
35

newline

42

newline

49

move to .cpp

src/modulemanager.cpp
91–92

newline

evpokp added a subscriber: evpokp.Nov 25 2018, 3:33 PM

Ping! Is this still being looked at? As distributions may phase out pulseaudio-gconf, this is important.

asturmlechner added inline comments.Jan 14 2019, 7:34 PM
CMakeLists.txt
44

this still needs to be addressed, anything else though?

nicolasfella marked 11 inline comments as done.
  • Use and reqire GSettings by default and add CMake option to use GConf
  • Coding style
drosca added inline comments.Mar 31 2019, 8:12 AM
src/gsettingsitem.cpp
59

= nullptr, otherwise you have a crash if the following switch falls to default case.

pino added a subscriber: pino.Mar 31 2019, 8:26 AM
pino added inline comments.
src/gsettingsitem.cpp
40

nullptr here

43

should this be qCCritical, to hard-fail in case other types of keys are used (instead of silently doing nothing)?

69

as above, qCCritical too?

drosca added inline comments.Mar 31 2019, 8:34 AM
src/gsettingsitem.cpp
43

qCritical is not hard-fail, that would be qFatal, but don't really see a reason to abort here in any case.

It should be at least qWarning though, as qDebug is usually disabled by default.

pino added inline comments.Mar 31 2019, 8:39 AM
src/gsettingsitem.cpp
43

As I said, to better not silently miss new types of keys, as logging to terminal will not help with a GUI application.

drosca added inline comments.Mar 31 2019, 8:47 AM
src/gsettingsitem.cpp
43

Sure, but qFatal here will take down the whole plasmashell and effectively break the login to desktop session.

Also, this should only be reached by developer when trying to read new key as type of existing keys should not change (and it already supports all types that are currently used in code).

  • address comments
drosca accepted this revision.Mar 31 2019, 2:58 PM
This revision is now accepted and ready to land.Mar 31 2019, 2:58 PM
This revision was automatically updated to reflect the committed changes.

Now that this is in, can we now finally fix https://gitlab.freedesktop.org/pulseaudio/pulseaudio/issues/509? Or is there yet more to do before that can happen?

broulik reopened this revision.Apr 4 2019, 4:25 PM

With this it now aborts here

(process:114351): GLib-GIO-ERROR **: 18:24:26.823: Settings schema 'org.freedesktop.pulseaudio.module-group' is not installed
This revision is now accepted and ready to land.Apr 4 2019, 4:25 PM
fvogt added a subscriber: fvogt.Apr 11 2019, 10:48 AM
kossebau added inline comments.May 14 2019, 6:53 PM
cmake/FindGIO.cmake
20

It seems _LibGIOLinkFlags, _LibGIOLinkDir & _LibGIOCflags are not propagated into the build. So if e.g. the libraries are not in default lib dir, linking will fail, cmp. https://build.kde.org/view/Failing/job/Plasma/job/plasma-pa/job/kf5-qt5%20FreeBSDQt5.12/18/

Please give this another look & also compare with other pkgconfig() using Find' macros.

nicolasfella closed this revision.May 15 2019, 2:27 PM