Bug 457951 - ksudoku commit 6e9d941ce with an existing theme config results in a blank play-space and qpainter engine errors
Summary: ksudoku commit 6e9d941ce with an existing theme config results in a blank pla...
Status: RESOLVED FIXED
Alias: None
Product: ksudoku
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Gentoo Packages Linux
: NOR normal
Target Milestone: ---
Assignee: Ian Wadham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-08-16 10:13 UTC by Duncan
Modified: 2022-08-17 21:19 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Debug logging patch for libkdegames master (c5848c0d673cebc67eb280d54081ca69efeb28b0) (1.57 KB, patch)
2022-08-16 13:35 UTC, Friedrich W. H. Kossebau
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Duncan 2022-08-16 10:13:07 UTC
On git-master (for frameworks/plasma/gear all three) using the gentoo/kde project overlay live-git ebuilds.  Current ksudoku HEAD 64ec262ee

For a couple months ksudoku would run, but attempting to actually play a game resulted in a blank play-space, with the number-selector space blank as well.

Running from konsole gave me this pair of errors, twice at the select game screen (which seemed to work), many many times when starting a game and getting the blank play-space:

QPainter::begin: Paint device returned engine == 0, type: 2
QPainter::end: Painter not active, aborted

I finally had some time to bisect it tonight, with the culprit being (emails masked):

commit 6e9d941ce
Author:     Friedrich W. H. Kossebau <kossebau@>
AuthorDate: Sun Jun 19 19:40:16 2022 +0200
Commit:     Friedrich W. H. Kossebau <kossebau@>
CommitDate: Sun Jun 19 19:40:16 2022 +0200

Port away from deprecated KGameTheme & KGameThemeSelector


Given that clue I rebuilt at HEAD, then tried reconfiguring my theme, and sure enough, ksudoku gave me a proper play-space (and no qpainter errors in konsole) once again. =:^)


So then I wondered what in the ksudokurc file it had been choking on.  A diff against a backup revealed this in the [Themes] section:

Old:
theme=themes/default.desktop

New:
theme=/usr/share/ksudoku/themes/default.desktop

So the code in the above commit does absolute paths while the old code apparently saved the path relative to the system theme location, and the new code doesn't know how to interpret that, resulting in a null theme, in turn resulting in the null/0 qpainter engine error above.
Comment 1 Duncan 2022-08-16 10:17:54 UTC
CCing kossebau@ as the author/committer of the culprit commit.
Comment 2 Friedrich W. H. Kossebau 2022-08-16 11:02:12 UTC
Hi, thanks for reporting the issue. Cannot remember I ran into such errors (though been some time) when I tested the change, and trying locally right now I also only get relative path with the new code when changing themes, like

theme=themes/abstraction.desktop

Will have a look in the code to see what condition might result in a full path later today.
Comment 3 Friedrich W. H. Kossebau 2022-08-16 12:10:04 UTC
From a first look for potential culprit code, it seems the issue is how the theme id is generated in the libkdegames classes used in the new code (KgThemeProvider):
`KgThemeProvider::rediscoverThemes()` goes over all `QStandardPaths::locateAll(QStandardPaths::AppDataLocation, "themes", QStandardPaths::LocateDirectory)` and collects the full path to the desktop files. Then it generates the theme id by first turning the desktop file path to a canonical version by `QFileInfo(file).canonicalFilePath()` and going once more over all `QStandardPaths::standardLocations(QStandardPaths::AppDataLocation)` and, if any of the latter path is a prefix to the canonical desktop file path, subtracts that prefix from the path to get the id. ((oh dear, how random people porting convoluted that logic :) )).
See https://invent.kde.org/games/libkdegames/-/blob/master/src/kgthemeprovider.cpp#L185 and https://invent.kde.org/games/libkdegames/-/blob/master/src/kgthemeprovider.cpp#L174

My theory is that the logic of that code seems to miss out some constellation on your system, where QStandardPaths calls deliver some paths which would be turned to something else by the idea of QFileInfo about what the canonical version is.

Are you capable to add some fprint debug lines to libkdegames yourself to get insight into the respective values used there?
Comment 4 Duncan 2022-08-16 13:14:57 UTC
(In reply to Friedrich W. H. Kossebau from comment #3)
> Then it generates the theme id by first turning the desktop file path
> to a canonical version 

That rings a bell.  My system has a reverse-usrmerge, /usr -> .  so everything installed to /usr ends up in /.  Up to probably about a year ago that was causing a problem with canonical/non-canonical path comparisons against the *.desktop file path that was supposed to give plasmashell the special privs it needs from kwin to get the wayland running tasks info (not a problem with insecure X), so none of the taskmanager plasmoids could actually see running tasks and all they were showing were the pinned launchers.  That problem turned out to be in whatever framework was doing the resolution, fixed when someone traced down an apparently unrelated instance of the same bug, and as soon as I saw the git log I knew what was going on with my bug and that it was probably fixed with that commit, which it was.

> Are you capable to add some fprint debug lines to libkdegames yourself to
> get insight into the respective values used there?

Give me a debug/test patch and I can apply it for local testing, no sweat, but coding it is another story... (Effectively I can and sometimes do hack existing patches when necessary, but I'm not a dev so coding my own, even for trivial debugging like this, if possible at all, typically takes me hours of bumbling.)
Comment 5 Friedrich W. H. Kossebau 2022-08-16 13:35:45 UTC
Created attachment 151355 [details]
Debug logging patch for libkdegames master (c5848c0d673cebc67eb280d54081ca69efeb28b0)
Comment 6 Friedrich W. H. Kossebau 2022-08-16 13:41:33 UTC
Please try the attached patch for libkdegames: apply, build, install. then start ksudoku on the commandline. Interesting will be all the log output starting with "THEMELOADING", should be something like (in my case having double installation in system and user self-built location):
--- 8< ---
THEMELOADING considering locations ([lots of paths], "/usr/share/ksudoku")
THEMELOADING considering desktop file "/home/some/path/share/ksudoku/themes/abstraction.desktop"
THEMELOADING canonical path "/home/some/path/share/ksudoku/themes/abstraction.desktop"
THEMELOADING id "themes/abstraction.desktop"
THEMELOADING considering desktop file "/home/some/path/share/ksudoku/themes/default.desktop"
THEMELOADING canonical path "/home/some/path/share/ksudoku/themes/default.desktop"
THEMELOADING id "themes/default.desktop"
THEMELOADING considering desktop file "/home/some/path/share/ksudoku/themes/ksudoku_scrible.desktop"
THEMELOADING canonical path "/home/some/path/share/ksudoku/themes/ksudoku_scrible.desktop"
THEMELOADING id "themes/ksudoku_scrible.desktop"
THEMELOADING considering desktop file "/usr/share/ksudoku/themes/abstraction.desktop"
THEMELOADING considering desktop file "/usr/share/ksudoku/themes/default.desktop"
THEMELOADING considering desktop file "/usr/share/ksudoku/themes/ksudoku_scrible.desktop"
--- 8< ---
Comment 7 Bug Janitor Service 2022-08-16 15:12:59 UTC
A possibly relevant merge request was started @ https://invent.kde.org/games/libkdegames/-/merge_requests/20
Comment 8 Duncan 2022-08-16 22:34:34 UTC
(In reply to Friedrich W. H. Kossebau from comment #6)
> Please try the attached patch for libkdegames
> Interesting will be all the log output starting with "THEMELOADING"

For clarity this is at libkdegames c5848c0d6 and $HOME=/h/x (with $XDG_CONFIG_HOME=$HOME/config), but having visited Egypt I /really/ like that theme and I haven't felt the need to HNS any themes, so (/usr)/share/ksudoku is the only location with any.

THEMELOADING considering locations ("/h/x/config/share/ksudoku", "/usr/local/share/ksudoku", "/usr/share/ksudoku")
THEMELOADING considering desktop file "/usr/share/ksudoku/themes/abstraction.desktop"
THEMELOADING canonical path "/share/ksudoku/themes/abstraction.desktop"
THEMELOADING id "/usr/share/ksudoku/themes/abstraction.desktop"
THEMELOADING considering desktop file "/usr/share/ksudoku/themes/default.desktop"
THEMELOADING canonical path "/share/ksudoku/themes/default.desktop"
THEMELOADING id "/usr/share/ksudoku/themes/default.desktop"
THEMELOADING considering desktop file "/usr/share/ksudoku/themes/ksudoku_scrible.desktop"
THEMELOADING canonical path "/share/ksudoku/themes/ksudoku_scrible.desktop"
THEMELOADING id "/usr/share/ksudoku/themes/ksudoku_scrible.desktop"
Comment 9 Friedrich W. H. Kossebau 2022-08-16 22:55:33 UTC
Thanks for getting and showing that log. Okay, that should back up the working theory about the cause :)

If you like to help more, please give the meanwhile created bug fix proposal as reported before by the bot a test. It should also apply to current master (I assume).

You can download the patch itself by blue "Code" button (choose "Plain diff" in the popup menu)  or using this link directly: https://invent.kde.org/games/libkdegames/-/merge_requests/20.diff
Comment 10 Duncan 2022-08-17 00:19:01 UTC
(In reply to Friedrich W. H. Kossebau from comment #9)
> https://invent.kde.org/games/libkdegames/-/merge_requests/20.diff

Good and bad news:
Good:   The relative style ksudokurc theme entry works with that patch.
Bad: The full-path style does not.

If there hasn't been a release with the now-existing code that creates the full-path entry I imagine the bad news can be ignored (unless we want to give people hand-editing their ksudokurc files the choice of doing full paths, too).

If there has been a release, the question is do we want to cover both styles and thus cover the short-period code that additionally only writes the full path for symlinked paths, or are we willing to sacrifice the narrow segment who both ran the new release and have in-path symlinks, most of who likely were upgrades already and thus had to do the blow-away/hand-edit process once already in ordered to get that full path entry in the first place?

Alternatively or as well, how difficult would it be to code up a "this entry doesn't make sense, ignore it and use/rewrite the default" solution?  I'd personally prefer that, as then the worst-case (triggered by say a fully corrupted entry or one pointed at a missing theme) would be losing the configured theme and resetting to default, instead of showing blank/broken game-play areas thus breaking the games in question.
Comment 11 Duncan 2022-08-17 00:26:33 UTC
(In reply to Duncan from comment #10)
> [H]ow difficult would it be to code up a "this entry
> doesn't make sense, ignore it and use/rewrite the default" solution?

... maybe with a warning, to STDERR, notification or dialog depending on evaluated priority of the user actually seeing it, saying the existing entry was corrupted (or whatever better wording) so it reverted to default.
Comment 12 Duncan 2022-08-17 00:52:59 UTC
Yikes!  The patch breaks kpat's Egyptian theme (at least with my existing kpatrc).  In the kpat case, however, it *does* revert to something else (default?) instead of blanking, as I proposed for ksudoku.  However, unlike ksudoku, I can't reset it to Egyptian -- it simply doesn't take -- tho I *can* set at least some other themes and they will take.  

It took all I tried (several, it ships way more than the three ksudoku ships) /except/ Egyptian.  Which is weird, as again, I've not HNS-ed additional themes so they're all as-shipped.  What makes kpat's Egyptian different so it won't take it?
Comment 13 Duncan 2022-08-17 01:33:10 UTC
(In reply to Duncan from comment #12)
> What makes kpat's Egyptian different so it won't take it?

User mistake.  I didn't read the tabs and was changing card decks instead of game themes, not realizing they were separate.  Once I changed game theme to Egyptian as well, it took.  I'm not entirely sure whether the card deck was changing to Egyptian all the time and I didn't notice it as I was staring at the (to me[1]) blinding green-blaze background, or whether it only took once I changed the game theme to Egyptian as well, but in any case, I can say the card deck Egyptian did take once I set the game theme Egyptian.

But we're still left with the fact that the patch breaks the existing kpatrc theme choice, tho not as badly as ksudoku's breakage, as kpat does at least revert to a usable default instead of blanking.   Both honor resetting the theme, but kpat's breakage is less as it reverts to a usable (if personally uncomfortable) default instead of breaking game play.

---
[1] I wear rigid gas-perm contacts with a rather high nearsightedness correction of ~ -10 diopters.  The combination of  the strongly negative correction and rigid lenses means too bright backgrounds star-flare-reflect in the contacts and eventually give me a headache as my eyes and brain try to compensate and see around/through the flare-reflections.   It took me years to figure out, but that's why I strongly prefer a "reversed" light-on-dark color-scheme as at least then the flares are smaller than when the entire background is bright.  The brown-sand Egyptian background is perfect for that, plus I visited and find the idea of the theme rather pleasing as well!
Comment 14 Friedrich W. H. Kossebau 2022-08-17 10:30:04 UTC
Thanks again for testing.

(In reply to Duncan from comment #10)
> (In reply to Friedrich W. H. Kossebau from comment #9)
> > https://invent.kde.org/games/libkdegames/-/merge_requests/20.diff
> 
> Good and bad news:
> Good:   The relative style ksudokurc theme entry works with that patch.

Okay :)

> Bad: The full-path style does not.
> 
> If there hasn't been a release with the now-existing code that creates the
> full-path entry I imagine the bad news can be ignored (unless we want to
> give people hand-editing their ksudokurc files the choice of doing full
> paths, too).

No release of ksudoko using the new code yet, though sadly 22.08.0 release is currently prepared for tomorrow release and would be the first version of ksudoko with the new code path. libkdegames has seen a quite some releases with the id screwing logic (was added in 2014).

I would agree that the most simple thing would be to just fallback to the default theme if the config holds a theme id that cannot be resolved.

Most places also already do that, so those games can at least be used for gaming. Though ksudoku needs a fix-up for handling that situation, blame on me here, no idea why I failed to consider bad config entries. Local patch already prepared and tested, landing next.

Will see to have the prepared 22.08.0 tarballs updated in time for tomorrow now.
Comment 15 Friedrich W. H. Kossebau 2022-08-17 10:30:59 UTC
Git commit fc9adcdd5eb843bc3024aa71787ea5de5950a14d by Friedrich W. H. Kossebau.
Committed on 17/08/2022 at 10:30.
Pushed by kossebau into branch 'release/22.08'.

Fix handling of invalid theme ids, fall back to default instead

M  +1    -1    src/gui/views/renderer.cpp

https://invent.kde.org/games/ksudoku/commit/fc9adcdd5eb843bc3024aa71787ea5de5950a14d
Comment 16 Friedrich W. H. Kossebau 2022-08-17 21:19:15 UTC
Git commit 91e246f2b17c899c1d5a9940d7b6f7d9893e2319 by Friedrich W. H. Kossebau.
Committed on 16/08/2022 at 15:10.
Pushed by kossebau into branch 'release/22.08'.

KgThemeProvider: fix generating broken full path theme id due to fs symlinks

relativeToApplications() was using QFileInfo(file).canonicalFilePath()
to compare against non-canonicalized directories returned by
QStandardPaths, thus missing out in case the file was in a path with
symlinks, resulting in ids with full absolute path.

This patch fixes that by calculating the id from the known one subdir name
and the desktop file name directly.

M  +1    -13   src/kgthemeprovider.cpp

https://invent.kde.org/games/libkdegames/commit/91e246f2b17c899c1d5a9940d7b6f7d9893e2319