D5627: Remove KDELibs4Support from KSudoku

[Johan Ouwerkerk]

ouwerkerk added a comment.


  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.

INLINE COMMENTS

> ksudoku.cpp:899
>  		KIO::file_copy (Url, QUrl::fromLocalFile(destDir + '/' + Url.fileName()));
>  	}

We should probably take the opportunity to tackle this here. 
The `QDir()` could be used to get a proper path that is safe to pass to `QUrl::fromLocalFile()`. 
Starting with the `QDir()` call above at line 886:

  QDir dest(destDir);
  dest.mkpath(dest.path());
   // some of the following code elided...
  KIO::file_copy( Url, QUrl::fromLocalFile(dest.filePath(Url.fileName())));

> serializer.cpp:402
>  
>  SKGraph *Serializer::loadCustomShape(const QUrl &url, QWidget* window, QString *errorMsg) {
> +	if ( url.isEmpty() ) return nullptr;

The body of this function is basically the same as the next one which returns a `Game` instead. Maybe we should extract and consolidate the bulk of this to a new shared function?

> serializer.cpp:465
> +	}
> +	if(tmpFile.open()) {
> +		int errorLine;

Is this safe? The docs for `QTemporaryFile` say:

> The file is guaranteed to have been created by this function (i.e., it has never existed before).

Therefore: is it possible that two successive calls to `.open()` refer to different temporary files? Wouldn't it be better to have a `const bool openedOK = tmpFile.open();` before the first check and then use that, as the `QTemporaryFile` is never closed in between?

> serializer.cpp:721
> +	{
> +		KMessageBox::error(window, i18n("Unable to upload file."), i18n("Error Writing File"));
> +	}

You should probably `return false;` here.

> symbols.cpp:41
>  	if(symbol == '.') return UNUSABLE;
>  	return c - 'a';

According to the `QChar` docs the result of trying `.toLatin1()` on something that isn't 'Latin 1' will be 0.
More generally depending on how this code path is reached, quite a lot of invalid 'Latin 1' chars exist and may be returned.
As a result trusting `c - 'a';` blindly may be a bit dangerous.

It's probably better to do a range check and if the character falls outside it return either `VACANT` or `UNUSABLE`.

REPOSITORY
  R417 KSudoku

REVISION DETAIL
  https://phabricator.kde.org/D5627

To: stikonas, #kde_games, ltoscano, ouwerkerk
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-games-devel/attachments/20170428/c25a95dc/attachment-0001.html>