Review Request 127952: add a function to override the current theme heuristics with a static value

[Harald Sitter]

> 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?
> 
> Harald Sitter wrote:
>     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.
> 
> Marco Martin wrote:
>     yeah, would be better for testing, the thing i'm a bit concerned on tough is that that function then can be used and abused outside tests too without it at least having a name that suggests it's only for tests.
>     maybe calling it forcedThemeForTests()?

+1


- Harald


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127952/#review95579
-----------------------------------------------------------


On May 18, 2016, 10:30 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, 10:30 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> 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 
>   autotests/kicontheme_unittest.cpp PRE-CREATION 
>   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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160518/911762aa/attachment-0001.html>