Looks basically sane, don't have enough knowledge of the real inner workings of ksudoku to offer much more feedback than this on the proposed changes.

However we should probably take the time investigate a couple of follow-up changes:

1. Certain serialisation/deserialisation functions may be consolidated into a couple of new shared functions to reduce the amount of duplicated logic there.
2. There is a somewhat oddly located "globals.h" which is included all over the place and smells. What to do about this?
3. KSudoku::updateShapesList() should be cleaned up, to not do hacky things with file paths when there is perfectly good Qt API for this already.

In D5627#105389, @ltoscano wrote:

Non-core sudokus are not loaded ("Unable to download file.") and then the program crash:

[KCrash Handler]
#6  0x000055b4c9440908 in QVector<int>::size() const ()
#7  0x000055b4c944f6a0 in ksudoku::Puzzle::hasSolution() const ()
#8  0x000055b4c945fafb in ksudoku::WelcomeScreen::generatePuzzle() ()
#9  0x000055b4c9460ebd in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (ksudoku::WelcomeScreen::*)()>::call(void (ksudoku::WelcomeScreen::*)(), ksudoku::WelcomeScreen*, void**) ()
#10 0x000055b4c9460e4f in void QtPrivate::FunctionPointer<void (ksudoku::WelcomeScreen::*)()>::call<QtPrivate::List<>, void>(void (ksudoku::WelcomeScreen::*)(), ksudoku::WelcomeScreen*, void**) ()
#11 0x000055b4c9460d5b in QtPrivate::QSlotObject<void (ksudoku::WelcomeScreen::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) ()

There may be other issues.

Looks like basically the same bug being tickled as with invalid QUrls. It seems to me there is a more fundamental memory management issue with Puzzles/Games that gets tickled if particular initialisation steps fail.
That is something we should investigate and fix more thoroughly later.

It does appear to be unused, OTOH its purpose appears to be to allow people to import new game variant types (shapes).
Logically that should have been wired up to a Get Hot New Stuff button/dialog for people to exchange custom puzzle variants.
Is that a feature we'd like to offer?

If I'm not mistaken, this is where the only call to that method was removed:
https://commits.kde.org/ksudoku/06cb470b229a49f714042f22094161a19ec26a73

It is not related to GHNS, thought, but about directly opening a local shape.

It is worth noting that the GHNS integration has been disabled at least since:
https://commits.kde.org/ksudoku/32e159a2a288fe9f991d85f36214c32917017010
(still in playground/games, so never worked in released versions by kde.org, only probably as standalone from sourceforge)

So: if you want to load a certain home-made Sudoku game, you can do it from File->Open (unless loadCustomShapeFromPath does something more). For more complex Sudoku, maybe yes, GHNS support could be useful, but wouldn't it be a different code anyway?

In D5627#105796, @ltoscano wrote:

unless loadCustomShapeFromPath does something more

That is precisely what it does ;)

1. It extracts data from an archive
2. And installs this to the the writable location of XDG_DATA_DIRS underneath a "ksudoku" directory
3. Then it calls updateShapesList();

updateShapesList(); is where we detect custom game variants by trawling through XDG_DATA_DIRS to find "ksudoku" directories with *.desktop files indicating custom game variants...
... My conclusion is that this function was meant for GHNS integration (or something similar to it) and for people to exchange custom puzzle types (game variants).

This is different from loading/saving an actual game or puzzle; this is about saving custom rules for *generating* custom puzzles.

EDIT to add:

Please note: I'm not categorically opposed to removing this functionality, but before we do so I'd like to be certain we are not removing something that we would consider the basis of a desirable feature. So to that end, perhaps the most relevant question is: is letting the user dynamically extend the repertoire of game variants by importing a package (archive) something we want to support in ksudoku? Specifically when we consider GHNS? Do we want to promote people designing custom sudoku variants or do we think so few people will bother it is better to support new variants via patches to the sudoku source (if at all)?

In D5627#105875, @ltoscano wrote:

I wonder if instead of all the if (), can't we just change the signatures to pass a real QString reference instead of a pointer?

Yes we can. In that case the callers need to be similar to:

const QString errorMsg;
const auto ptr = doTheCall(..., errorMsg);
if(!ptr) {
  KMessageBox ....
}

Do we really need the auto pointer? (just to check if that would bump some requirement, maybe just for one usage is not needed)

In D5627#105879, @ltoscano wrote:

Do we really need the auto pointer? (just to check if that would bump some requirement, maybe just for one usage is not needed)

Of course not: in each case the appropriate type should be used instead. ;)

For load() the specific check is a bit different:

const QString errorMsg;
const Game game = doTheCall(... errorMsg)
if(!game.puzzle()) {
  KMessageBox ...
}

In D5627#105881, @ouwerkerk wrote:

In D5627#105879, @ltoscano wrote:

Do we really need the auto pointer? (just to check if that would bump some requirement, maybe just for one usage is not needed)

Of course not: in each case the appropriate type should be used instead. ;)

For load() the specific check is a bit different:

const QString errorMsg;
const Game game = doTheCall(... errorMsg)
if(!game.puzzle()) {
  KMessageBox ...
}

I think the QString shouldn't be const. We want to modify it inside load.

In D5627#105882, @stikonas wrote:

I think the QString shouldn't be const. We want to modify it inside load.

You are, of course, entirely correct.