Differential D10259

[Look and feel KCM] Fix copying color scheme data
ClosedPublic
Actions

Authored by kossebau on Feb 3 2018, 1:17 AM.

Details

Reviewers
broulik
davidedmundson
mart
Commits
R119:6ab44dcb86b1: [Look and feel KCM] Fix copying color scheme data
Summary

Using one argument with "KSharedConfig::openConfig(filename)" actually
results in using the default value for the second argument, "FullConfig".
Which results also in "Blend kdeglobals into the config object."
As a result the color scheme file opened in KCMLookandFeel::setColors(...)
this way has the complete global config groups and settings (with the data
as stored on the disc, not in any runtime working copies) mapped into the
profile (incl. e.g. the group "[General]" with the entry "ColorScheme").
So when then all the groups are copied over from the scheme config, actually
also all the stored global config groups and settings are copied over,
overwriting the not yet synced [General]/ColorScheme entry, which was just
set a few lines before, again with the old value from the storage.

Possibly the old code which was doing the sync right after setting the new
scheme value had been there exactly to protect against that
(cmp. change in cf49d415e7bb30e98c0e7529e7307d8449b8ffcd).

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Feb 3 2018, 1:17 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 3 2018, 1:17 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
kossebau requested review of this revision.Feb 3 2018, 1:17 AM
kossebau added a comment.Feb 3 2018, 1:21 AM

Another option would be to move the

configGroup.writeEntry("ColorScheme", scheme);

behind the config-copy loop.

But then I would think that we do not want to blend in the kdeglobals config into the color schemes files data in general, or? There is similar color scheme loading code below "kcms/colors/", where also the kdeglobals are blended in. Those might need to be fixed as well then.

kossebau edited the summary of this revision. (Show Details)Feb 3 2018, 1:22 AM
kossebau added a comment.Feb 3 2018, 1:25 AM

Oh, and this fixes the unit test "KcmTest::testKCMSave()" which is failing since cf49d415e7bb30e98c0e7529e7307d8449b8ffcd (both in 5.12 and master after yesterday's merge).

kossebau added a comment.Feb 8 2018, 6:46 PM

Okay, got my test user setup working again and thus finally can test how the code behaves in real life, next to the unit test.

Turns out that the "bug" is not opposed to the user by some magic things happening. In the normal usage the new setting is somehow synced before the color profile is loaded with the global settings mapped into (ex: activating the Oxygen LnF theme)

BEFORE cs "Breeze"
WRITING cs "Oxygen"
READING cs "Oxygen"

, while in the unit test this does not happen (as triggered e.g. by KcmTest::testKCMSave())

QDEBUG : KcmTest::testKCMSave() BEFORE cs "customTestValue"
QDEBUG : KcmTest::testKCMSave() WRITING cs "TestValue"
QDEBUG : KcmTest::testKCMSave() READING cs "customTestValue"

Debug output created by these lines added:

    KConfigGroup configGroup(&m_config, "General");
qDebug() << "BEFORE cs" << configGroup.readEntry("ColorScheme"); // added
    configGroup.writeEntry("ColorScheme", scheme);
qDebug() << "WRITING cs" << configGroup.readEntry("ColorScheme"); // added

    KSharedConfigPtr conf = KSharedConfig::openConfig(colorFile);//, KSharedConfig::CascadeConfig);
{  // added
KConfigGroup cg(conf, "General");  // added
qDebug() << "READING cs" << cg.readEntry("ColorScheme");  // added
}  // added

No explanation yet found for the different behaviour,

In any case, more important are these debug lines, which show off that without KSharedConfig::CascadeConfig and thus with the mapped global config settings, the color scheme loop copies not only the color scheme, but also all the global config (each line logging "group (keys)"):

COLORSCHEME "Colors:View" ("BackgroundAlternate", "BackgroundNormal", "DecorationFocus", "DecorationHover", "ForegroundActive", "ForegroundInactive", "ForegroundLink", "ForegroundNegative", "ForegroundNeutral", "ForegroundNormal", "ForegroundPositive", "ForegroundVisited")
COLORSCHEME "WM" ("activeBackground", "activeBlend", "activeFont", "activeForeground", "inactiveBackground", "inactiveBlend", "inactiveForeground")
COLORSCHEME "ColorEffects:Disabled" ("Color", "ColorAmount", "ColorEffect", "ContrastAmount", "ContrastEffect", "IntensityAmount", "IntensityEffect")
COLORSCHEME "Colors:Selection" ("BackgroundAlternate", "BackgroundNormal", "DecorationFocus", "DecorationHover", "ForegroundActive", "ForegroundInactive", "ForegroundLink", "ForegroundNegative", "ForegroundNeutral", "ForegroundNormal", "ForegroundPositive", "ForegroundVisited")
COLORSCHEME "ColorEffects:Inactive" ("ChangeSelectionColor", "Color", "ColorAmount", "ColorEffect", "ContrastAmount", "ContrastEffect", "Enable", "IntensityAmount", "IntensityEffect")
COLORSCHEME "PreviewSettings" ("MaximumRemoteSize", "camera", "file", "fonts")
COLORSCHEME "Colors:Button" ("BackgroundAlternate", "BackgroundNormal", "DecorationFocus", "DecorationHover", "ForegroundActive", "ForegroundInactive", "ForegroundLink", "ForegroundNegative", "ForegroundNeutral", "ForegroundNormal", "ForegroundPositive", "ForegroundVisited")
COLORSCHEME "KShortcutsDialog Settings" ("Dialog Size")
COLORSCHEME "KFileDialog Settings" ("Automatically select filename extension", "Breadcrumb Navigation", "Decoration position", "LocationCombo Completionmode", "PathCombo Completionmode", "Preview Width", "Previews", "Show Bookmarks", "Show Full Path", "Show Preview", "Show Speedbar", "Show hidden files", "Sort by", "Sort directories first", "Sort reversed", "Speedbar Width", "View Style", "detailedViewIconSize", "listViewIconSize")
COLORSCHEME "Appmenu Style" ("Style")
COLORSCHEME "DialogIcons" ("ActiveColor", "ActiveColor2", "ActiveEffect", "ActiveSemiTransparent", "ActiveValue", "Animated", "DefaultColor", "DefaultColor2", "DefaultEffect", "DefaultSemiTransparent", "DefaultValue", "DisabledColor", "DisabledColor2", "DisabledEffect", "DisabledSemiTransparent", "DisabledValue", "Size")
COLORSCHEME "MainToolbarIcons" ("ActiveColor", "ActiveColor2", "ActiveEffect", "ActiveSemiTransparent", "ActiveValue", "Animated", "DefaultColor", "DefaultColor2", "DefaultEffect", "DefaultSemiTransparent", "DefaultValue", "DisabledColor", "DisabledColor2", "DisabledEffect", "DisabledSemiTransparent", "DisabledValue", "Size")
COLORSCHEME "KDE" ("ChangeCursor", "ColorScheme", "LookAndFeelPackage", "ShowIconsInMenuItems", "ShowIconsOnPushButtons", "contrast", "widgetStyle")
COLORSCHEME "Toolbar style" ("ToolButtonStyle", "ToolButtonStyleOtherToolbars")
COLORSCHEME "ToolbarIcons" ("ActiveColor", "ActiveColor2", "ActiveEffect", "ActiveSemiTransparent", "ActiveValue", "Animated", "DefaultColor", "DefaultColor2", "DefaultEffect", "DefaultSemiTransparent", "DefaultValue", "DisabledColor", "DisabledColor2", "DisabledEffect", "DisabledSemiTransparent", "DisabledValue", "Size")
COLORSCHEME "DesktopIcons" ("ActiveColor", "ActiveColor2", "ActiveEffect", "ActiveSemiTransparent", "ActiveValue", "Animated", "DefaultColor", "DefaultColor2", "DefaultEffect", "DefaultSemiTransparent", "DefaultValue", "DisabledColor", "DisabledColor2", "DisabledEffect", "DisabledSemiTransparent", "DisabledValue", "Size")
COLORSCHEME "DirSelect Dialog" ("DirSelectDialog Size", "History Items")
COLORSCHEME "SmallIcons" ("ActiveColor", "ActiveColor2", "ActiveEffect", "ActiveSemiTransparent", "ActiveValue", "Animated", "DefaultColor", "DefaultColor2", "DefaultEffect", "DefaultSemiTransparent", "DefaultValue", "DisabledColor", "DisabledColor2", "DisabledEffect", "DisabledSemiTransparent", "DisabledValue", "Size")
COLORSCHEME "KDE URL Restrictions" ("rule_1", "rule_count")
COLORSCHEME "Paths" ("Trash")
COLORSCHEME "Colors:Complementary" ("BackgroundAlternate", "BackgroundNormal", "DecorationFocus", "DecorationHover", "ForegroundActive", "ForegroundInactive", "ForegroundLink", "ForegroundNegative", "ForegroundNeutral", "ForegroundNormal", "ForegroundPositive", "ForegroundVisited")
COLORSCHEME "Directories" ("dir_pixmap")
COLORSCHEME "Icons" ("Theme")
COLORSCHEME "KisShortcutsDialog Settings" ("Dialog Size")
COLORSCHEME "PanelIcons" ("ActiveColor", "ActiveColor2", "ActiveEffect", "ActiveSemiTransparent", "ActiveValue", "Animated", "DefaultColor", "DefaultColor2", "DefaultEffect", "DefaultSemiTransparent", "DefaultValue", "DisabledColor", "DisabledColor2", "DisabledEffect", "DisabledSemiTransparent", "DisabledValue", "Size")
COLORSCHEME "Colors:Window" ("BackgroundAlternate", "BackgroundNormal", "DecorationFocus", "DecorationHover", "ForegroundActive", "ForegroundInactive", "ForegroundLink", "ForegroundNegative", "ForegroundNeutral", "ForegroundNormal", "ForegroundPositive", "ForegroundVisited")
COLORSCHEME "General" ("BrowserApplication", "ColorScheme", "Name", "dbfile", "desktopFont", "fixed", "font", "menuFont", "shadeSortColumn", "smallestReadableFont", "taskbarFont", "toolBarFont", "widgetStyle")
COLORSCHEME "Colors:Tooltip" ("BackgroundAlternate", "BackgroundNormal", "DecorationFocus", "DecorationHover", "ForegroundActive", "ForegroundInactive", "ForegroundLink", "ForegroundNegative", "ForegroundNeutral", "ForegroundNormal", "ForegroundPositive", "ForegroundVisited")
COLORSCHEME "Translations" ("LANGUAGE")

where with the setting KSharedConfig::CascadeConfig it is just the color scheme data, as desired;

COLORSCHEME "ColorEffects:Disabled" ("Color", "ColorAmount", "ColorEffect", "ContrastAmount", "ContrastEffect", "IntensityAmount", "IntensityEffect")
COLORSCHEME "Colors:Complementary" ("BackgroundAlternate", "BackgroundNormal", "DecorationFocus", "DecorationHover", "ForegroundActive", "ForegroundInactive", "ForegroundLink", "ForegroundNegative", "ForegroundNeutral", "ForegroundNormal", "ForegroundPositive", "ForegroundVisited")
COLORSCHEME "ColorEffects:Inactive" ("ChangeSelectionColor", "Color", "ColorAmount", "ColorEffect", "ContrastAmount", "ContrastEffect", "Enable", "IntensityAmount", "IntensityEffect")
COLORSCHEME "Colors:View" ("BackgroundAlternate", "BackgroundNormal", "DecorationFocus", "DecorationHover", "ForegroundActive", "ForegroundInactive", "ForegroundLink", "ForegroundNegative", "ForegroundNeutral", "ForegroundNormal", "ForegroundPositive", "ForegroundVisited")
COLORSCHEME "KDE" ("contrast")
COLORSCHEME "Colors:Window" ("BackgroundAlternate", "BackgroundNormal", "DecorationFocus", "DecorationHover", "ForegroundActive", "ForegroundInactive", "ForegroundLink", "ForegroundNegative", "ForegroundNeutral", "ForegroundNormal", "ForegroundPositive", "ForegroundVisited")
COLORSCHEME "WM" ("activeBackground", "activeBlend", "activeForeground", "inactiveBackground", "inactiveBlend", "inactiveForeground")
COLORSCHEME "Colors:Button" ("BackgroundAlternate", "BackgroundNormal", "DecorationFocus", "DecorationHover", "ForegroundActive", "ForegroundInactive", "ForegroundLink", "ForegroundNegative", "ForegroundNeutral", "ForegroundNormal", "ForegroundPositive", "ForegroundVisited")
COLORSCHEME "Colors:Tooltip" ("BackgroundAlternate", "BackgroundNormal", "DecorationFocus", "DecorationHover", "ForegroundActive", "ForegroundInactive", "ForegroundLink", "ForegroundNegative", "ForegroundNeutral", "ForegroundNormal", "ForegroundPositive", "ForegroundVisited")
COLORSCHEME "Colors:Selection" ("BackgroundAlternate", "BackgroundNormal", "DecorationFocus", "DecorationHover", "ForegroundActive", "ForegroundInactive", "ForegroundLink", "ForegroundNegative", "ForegroundNeutral", "ForegroundNormal", "ForegroundPositive", "ForegroundVisited")
COLORSCHEME "General" ("ColorScheme", "Name", "shadeSortColumn")

Debug output created by this line added:

    foreach (const QString &grp, conf->groupList()) {
        KConfigGroup cg(conf, grp);
qDebug() << "COLORSCHEME" << grp << cg.keyList(); // added
        KConfigGroup cg2(&m_config, grp);
        cg.copyTo(&cg2);
    }

So even if by chance(?) not resulting in a bug for the user, still the old code does unneeded things. And triggers the fail of the unit test KcmTest::testKCMSave().

So IMHO this is a patch good to have in. And similarly also to be applied for other places which load color schemes in a KSharedConfig object.

broulik accepted this revision.Feb 15 2018, 8:42 PM

Looks good to me but I don't feel confident enough to give it a shipit but if it fixes the unittest and works..

This revision is now accepted and ready to land.Feb 15 2018, 8:42 PM
Closed by commit R119:6ab44dcb86b1: [Look and feel KCM] Fix copying color scheme data (authored by kossebau). · Explain WhyFeb 16 2018, 12:05 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mkcms/lookandfeel/kcm.cpp (2 lines)

Diff 27313

View Options

kcms/lookandfeel/kcm.cpp

Loading...