D11034: CoreApplicationSettingsTest

Johnny Jazeix noreply at phabricator.kde.org
Mon Mar 5 07:00:08 UTC 2018


jjazeix added inline comments.

INLINE COMMENTS

> ApplicationSettingsTest.cpp:92
> +{
> +	ApplicationSettings* applicationsettings = new ApplicationSettings(nullptr);
> +	// Setting Up null is required as it may already be present in memory because of unsuccessful test/ interruption

applicationSettings

> ApplicationSettingsTest.cpp:93
> +	ApplicationSettings* applicationsettings = new ApplicationSettings(nullptr);
> +	// Setting Up null is required as it may already be present in memory because of unsuccessful test/ interruption
> +	applicationsettings->setFullscreen(NULL);

It would be better to override the class with a constructor that initialize these fields.
The most important part is that on this file, we are writing on the configuration. You also need to override it to not change the real configuration (https://github.com/gcompris/GCompris-qt/blob/ba315944567ada9440b95f798887dbdc408074e6/src/core/ApplicationSettings.cpp#L92)

It would be nice to also test the content of the configuration file at the end of the function to be sure all the content has been written as expected.

REPOSITORY
  R2 GCompris

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

To: himanshuvishwakarma, jjazeix, dmadaan, rudranilbasu, timotheegiet
Cc: #kde_edu, #gcompris, harrymecwan, ganeshredcobra, nityanandkumar, echarruau, rahulyadav, narvaez, scagarwal, apol, timotheegiet, hkaelberer, jjazeix, bcoudoin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20180305/9f61c558/attachment-0001.html>


More information about the kde-edu mailing list