Review Request 126001: In KConfigTest::testEntryMap, convert QByteArray with nulls using a char *

[Matthew Dawson]

> On Nov. 9, 2015, 2:57 a.m., David Faure wrote:
> > Isn't this exactly the wrong fix? The goal of the test with embedded NULs in a QByteArray is to test that KConfig can be used to store binary data, isn't it? So I would think the expected value was correct, it's the KConfig code that is faulty. As a programmer I expect to be able to write binary data and read it back again, without it stopping at the first \0.

I'm not exactly sure what this test is trying to do, or what the purpose of the KConfig::entryMap function is.  KConfig::entryMap only returns QString (not QByteArray, or any other type), so its value seems limited when I can get the same behaviour through other methods.  There is a test that looks at the raw byte string to ensure nulls are passed through correctly (see kconfigtest.cpp, line 343), so changing the behaviour just to test nulls isn't useful.

What I'm not sure of is what someone would expect when asking for a QString from a value in KConfig containing nulls.  If someone is thinking like a C programmer, I'd expect them to expect the string to end at the null character.  Otherwise, I can see the argument returning the string with nulls embedded in at as being the expected outcome.  I think either way, people will end up suprised.

I'd also expect the KConfigGroup::readEntry to return the same value as what KConfig::entryMap returns.  Currently readEntry also stops at the first null character.

I'm not aware of a preference in the Qt/KF5 world of how to deal with nulls in a string.  If there is a preference either way, I'm happy to take that to fit in with the rest of the ecosystem.


Note: none of this changes how a QByteArray should work, as I'd expect that to return the exact binary data you feed into it.  If it stopped processing at the first 0 byte, I'd consider that to be a bug regardless of how QString is handled.


- Matthew


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


On Nov. 8, 2015, 7:23 p.m., Matthew Dawson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126001/
> -----------------------------------------------------------
> 
> (Updated Nov. 8, 2015, 7:23 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> Due to https://codereview.qt-project.org/#/c/106473/, Qt 5.6 keeps null
> characters in QByteArray -> QString conversions, which breaks this test as
> one QByteArray contains nulls.  Instead, convert the QByteArray to const
> char * first, so QString stops at the first null.
> 
> The actual  behaviour of KConfig is unchanged, as internally the conversion
> always went through a const char *, which avoids creating QStrings with
> null characters.
> 
> 
> Diffs
> -----
> 
>   autotests/kconfigtest.cpp 9a2998647b5e5f54d63059172b727505a8ae1c80 
> 
> Diff: https://git.reviewboard.kde.org/r/126001/diff/
> 
> 
> Testing
> -------
> 
> Tested on both Qt 5.5.1 and Qt 5.6 (commit e996d68f6130847637ba287518cff1289cfa48e5), tests all pass now.
> 
> 
> Thanks,
> 
> Matthew Dawson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20151109/7dfdfd07/attachment.html>