> On May 18, 2016, 9:58 a.m., Marco Martin wrote: > > src/kicontheme.h, line 223 > > <https://git.reviewboard.kde.org/r/127952/diff/1/?file=465111#file465111line223> > > > > what about just a testmode bool like qstandardpaths?
That would be the other approach I talk about in the description. I am fairly indifferent which of the two approaches we use. This one is certainly more to the point as it ultimately allows getting rid of the entire kdeglobals hackery in tests. ::forceCurrent is functionally equal to test env setup like this: ``` KConfigGroup cg(KSharedConfig::openConfig(), "Icons"); cg.writeEntry("Theme", "test-theme"); cg.sync(); KSharedConfig::openConfig()->reparseConfiguration(); ``` In fact what we could if we add a new function to KIconLoader do is have the following: ``` KIconTheme::forceCurrent("test-theme"); KIconLoader::global()->fullReload(); ``` to be equal to what we have now in plasma tests: ``` KConfigGroup cg(KSharedConfig::openConfig(), "Icons"); cg.writeEntry("Theme", "test-theme"); cg.sync(); KSharedConfig::openConfig()->reparseConfiguration(); KIconTheme::reconfigure(); KIconLoader::global()->reconfigure(QString()); ``` So, the way I see it a generic ::setTestingModeEnabled would only solve the issue at hand of QIcon::themeName interfering with expectations, while with the presented ::forceCurrent we can solve the issue at hand and reduce overhead code in testing. - Harald ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127952/#review95579 ----------------------------------------------------------- On May 18, 2016, 8:25 a.m., Harald Sitter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127952/ > ----------------------------------------------------------- > > (Updated May 18, 2016, 8:25 a.m.) > > > Review request for KDE Frameworks, Christoph Feck, David Edmundson, and Marco > Martin. > > > Repository: kiconthemes > > > Description > ------- > > this allows unit tests to force a specific name to be used regardless of > which branch of the current() resolution would bite first. the override > wins and that is that. > > in particular this allows unit tests to force a fake theme > > > NOTES: > - this approach means tests need to be explicitly adapted to this new thing > (e.g. https://paste.kde.org/pjzwg47s7) > - another approach would be to have a switch to disable qicon::theme > entirely, which has the advantage of not requiring additional changes to > other tests > - the presented approach in the long run (with possibly a bit more rigging in > kiconloader) allow a test to force all it's kiconthemes magic to use a given > theme without having to touch kdeglobals or running manually reconfigures etc. > - both viable, I like this one better though > - ::overrideCurrent would be another possible name but ::force sounds more > like what it ultimately does in terms of impact > > > Diffs > ----- > > autotests/CMakeLists.txt 7af101398774bd138fe57f3765416cfcfb022f75 > src/kicontheme.h dafece91f401ea0f8188613c55ee92d96d293879 > src/kicontheme.cpp f3d4cf71c05fe143464952bec89497fb911f2291 > > Diff: https://git.reviewboard.kde.org/r/127952/diff/ > > > Testing > ------- > > unit test + adapted plasma-framework's themetest to use this > > > Thanks, > > Harald Sitter > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel