<br><br><div><span class="gmail_quote">On 10/28/07, <b class="gmail_sendername">Thomas Braxton</b> <<a href="mailto:kde.braxton@gmail.com">kde.braxton@gmail.com</a>> wrote:</span><blockquote class="gmail_quote" style="margin:0;margin-left:0.8ex;border-left:1px #ccc solid;padding-left:1ex">
<br><br><div><span class="q"><span class="gmail_quote">On 10/28/07, <b class="gmail_sendername">Oswald Buddenhagen</b> <<a href="mailto:ossi@kde.org" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)">
ossi@kde.org</a>> wrote:</span><blockquote class="gmail_quote" style="margin:0;margin-left:0.8ex;border-left:1px #ccc solid;padding-left:1ex">
On Sun, Oct 28, 2007 at 07:19:19AM -0400, Jeff Mitchell wrote:<br>> On Sunday 28 October 2007, Thomas Braxton wrote:<br>> > On 10/28/07, Jeff Mitchell <<a href="mailto:kde-dev@emailgoeshere.com" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)">
kde-dev@emailgoeshere.com
</a>> wrote:<br>> > > There is a [PortableDevices] section that is occasionally added to<br>> > > or deleted from. The problem is that whenever the last<br>> > > (positionally, not numerically) entry is removed, it's not
<br>> > > actually removed, but rather gets corrupted somehow.<br>> ><br>> > actually is not getting corrupted, it's being marked as deleted.<br>><br>> Why actually delete some, and only mark others?
<br>><br>if there is no default, there is no point in overshadowing it.<br>if you don't understand it, revisit the cascading config concept.<br><br>> > Is there a default set for those keys in KDEDIR/share/config/amarokrc?
<br>> > because the entries shouldn't be marked as deleted unless there was a<br>> > default, I'll look into this.<br>><br>> No, there isn't. No defaults.<br>><br>maybe a real bug then. thomas, your turn. :-P
</blockquote><div><br> </div></span><div>yeah a bug, I'm still trying to figure out why it's treating some entries as having defaults when they don't. It's in the backend when the entries are actually written to disk. I have a fix, but it's not quite right, it breaks something else, something wierd happening here. I'll try to get this fixed today.
</div></div></blockquote><div><br class="webkit-block-placeholder"></div><div>Ok, I've had time to work on this. This patch fixes the wierdness and removes entries from other locales from the entryMap, since I was already in the area ;) I also added a test, since the test for "stringEntry1[fr]" was wrong since KConfigGroup was saving the key as "stringEntry1\x5cfr\x5d", I think those are the right codes. So take a look and let me know what you think.
</div><div><br class="webkit-block-placeholder"></div><div> Index: tests/kconfigtest.cpp</div><div>===================================================================</div><div>--- tests/kconfigtest.cpp<span class="Apple-tab-span" style="white-space:pre">
</span>(revision 730047)</div><div>+++ tests/kconfigtest.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -90,7 +90,6 @@</div><div> cg.writeEntry( ESCAPEKEY, ESCAPEENTRY );</div>
<div> cg.writeEntry( "emptyEntry", "");</div><div> cg.writeEntry( "stringEntry1", STRINGENTRY1 );</div><div>- cg.writeEntry( "stringEntry1[fr]", TRANSLATEDSTRINGENTRY1 );</div>
<div> cg.writeEntry( "stringEntry2", STRINGENTRY2 );</div><div> cg.writeEntry( "stringEntry3", STRINGENTRY3 );</div><div> cg.writeEntry( "stringEntry4", STRINGENTRY4 );</div><div>@@ -258,6 +257,36 @@
</div><div> QCOMPARE( sc3.readEntry( "doubleEntry1", 0.0 ), DOUBLEENTRY );</div><div> }</div><div> </div><div>+void KConfigTest::testLocale()</div><div>+{</div><div>+ KConfig config("kconfigtest");
</div><div>+ config.setLocale("fr");</div><div>+ KConfigGroup group = config.group("Hello");</div><div>+ group.writeEntry("stringEntry1", QString(TRANSLATEDSTRINGENTRY1), KConfig::Localized|KConfig::Persistent);
</div><div>+ config.sync();</div><div>+</div><div>+ int count=0;</div><div>+ foreach(const QByteArray& line, readLines())</div><div>+ if (line.startsWith("stringEntry1[fr]"))</div><div>+ count++;
</div><div>+ QVERIFY(count == 1);</div><div>+ QCOMPARE(group.readEntry("stringEntry1", QString()), QString(TRANSLATEDSTRINGENTRY1));</div><div>+</div><div>+ config.setLocale("C"); // strings written in the "C" locale are written as nonlocalized
</div><div>+ group.writeEntry("stringEntry1", QString(STRINGENTRY1), KConfig::Localized|KConfig::Persistent);</div><div>+ config.sync();</div><div>+</div><div>+ count=0;</div><div>+ foreach(const QByteArray& line, readLines()) {
</div><div>+ if (line.startsWith("stringEntry1"))</div><div>+ count++;</div><div>+ if (line.contains("[C]"))</div><div>+ count--;</div><div>+ }</div><div>+ QVERIFY(count == 1);
</div><div>+ QCOMPARE(group.readEntry("stringEntry1", QString()), QString(STRINGENTRY1));</div><div>+}</div><div>+</div><div> void KConfigTest::testLists()</div><div> {</div><div> KConfig sc2( "kconfigtest" );
</div><div>@@ -388,8 +417,6 @@</div><div> QMap<QString, QString> entryMap = cg.entryMap();</div><div> qDebug() << entryMap.keys();</div><div> QCOMPARE(entryMap.value("stringEntry1"), QString(STRINGENTRY1));
</div><div>- // #### Why is this translated entry here? This just bloats the entryMap.</div><div>- QCOMPARE(entryMap.value("stringEntry1[fr]"), QString(TRANSLATEDSTRINGENTRY1));</div><div> QCOMPARE(entryMap.value
("stringEntry2"), QString(STRINGENTRY2));</div><div> QCOMPARE(entryMap.value("stringEntry3"), QString(STRINGENTRY3));</div><div> QCOMPARE(entryMap.value("stringEntry4"), QString(STRINGENTRY4));
</div><div>@@ -500,6 +527,23 @@</div><div> QVERIFY( sc.entryMap("AAA").isEmpty() );</div><div> QVERIFY( !sc.entryMap("Hello").isEmpty() ); //not deleted group</div><div> QVERIFY( sc.entryMap("FooBar").isEmpty() ); //inexistant group
</div><div>+ </div><div>+ // test for entries that are marked as deleted when there is no default</div><div>+ KConfig config("kconfigtest", KConfig::SimpleConfig); // make sure there are no defaults</div><div>
+ cg = config.group("Portable Devices");</div><div>+ cg.writeEntry("devices|manual|(null)", "whatever");</div><div>+ cg.writeEntry("devices|manual|/mnt/ipod", "/mnt/ipod");
</div><div>+ cg.sync();</div><div>+ </div><div>+ int count=0;</div><div>+ foreach(const QByteArray& item, readLines())</div><div>+ if (item.startsWith("devices|"))</div><div>+ count++;</div>
<div>+ QVERIFY(count == 2);</div><div>+ cg.deleteEntry("devices|manual|/mnt/ipod");</div><div>+ cg.sync();</div><div>+ foreach(const QByteArray& item, readLines())</div><div>+ QVERIFY(item != "devices|manual|/mnt/ipod[$d]");
</div><div> }</div><div> </div><div> void KConfigTest::testDefaultGroup()</div><div>Index: tests/kconfigtest.h</div><div>===================================================================</div><div>--- tests/kconfigtest.h
<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 730047)</div><div>+++ tests/kconfigtest.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -36,6 +36,7 @@</div>
<div> void initTestCase();</div><div> void cleanupTestCase();</div><div> void testSimple();</div><div>+ void testLocale();</div><div> void testLists();</div><div> void testPath();</div><div> void testPersistenceOfExpandFlagForPath();
</div><div>Index: config/kconfigdata.h</div><div>===================================================================</div><div>--- config/kconfigdata.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 730047)
</div><div>+++ config/kconfigdata.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -302,6 +302,18 @@</div><div> {</div><div> return getEntryOption(findEntry(group, key, flags), option);
</div><div> }</div><div>+ void removeEntry(const KEntryKey& key)</div><div>+ {</div><div>+ remove(key);</div><div>+ int count=0;</div><div>+ foreach(const KEntryKey& item, keys())
</div><div>+ if(item.mGroup == key.mGroup && !item.mKey.isEmpty())</div><div>+ count++;</div><div>+ if (!count) {</div><div>+ const KEntryKey groupKey(key.mGroup
);</div><div>+ remove(groupKey);</div><div>+ }</div><div>+ }</div><div> </div><div> void setEntryOption(Iterator it, EntryOption option, bool bf)</div><div> {</div><div>Index: config/kconfigini.cpp
</div><div>===================================================================</div><div>--- config/kconfigini.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 730047)</div><div>+++ config/kconfigini.cpp
<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -202,10 +202,8 @@</div><div> if (!locale.isEmpty()) {</div><div> if (locale != currentLocale) {</div><div>
// backward compatibility. C == en_US</div><div>- if (locale.at(0) != 'C' || currentLocale != "en_US") {</div><div>- aKey += '[' + locale + ']';
</div><div>- locale = QByteArray();</div><div>- }</div><div>+ if (locale.at(0) != 'C' || currentLocale != "en_US")</div><div>+ goto next_line;
</div><div> }</div><div> }</div><div> </div><div>@@ -234,28 +232,11 @@</div><div> if ((key.mGroup != "<default>") == defaultGroup)</div><div> continue; // skip
</div><div> </div><div>- // Skip default values and group headers</div><div>- if (key.bDefault || key.mKey.isNull())</div><div>+ // Skip group headers</div><div>+ if (key.mKey.isNull())</div><div>
continue; // skip</div><div> </div><div> const KEntry& currentEntry = *it;</div><div>- KEntryMapConstIterator testIt = it;</div><div>- ++testIt;</div><div>- if (testIt != end) { // might have a default
</div><div>- const KEntryKey& defaultKey = testIt.key();</div><div>- if (defaultKey.bDefault ||</div><div>- defaultKey.mGroup != key.mGroup ||</div><div>- defaultKey.mKey
!= key.mKey ||</div><div>- defaultKey.bLocal != key.bLocal) {</div><div>- if (currentEntry.bDeleted)</div><div>- continue; // Don't write deleted entries if there is no default
</div><div>- } else {</div><div>- if (currentEntry.mValue == testIt->mValue &&</div><div>- currentEntry.bDeleted == testIt->bDeleted)</div><div>- continue; // same as default, don't write
</div><div>- }</div><div>- }</div><div>-</div><div> if (!defaultGroup && currentGroup != key.mGroup) {</div><div> if (!firstEntry)</div><div> file.putChar('\n');
</div><div>@@ -270,7 +251,7 @@</div><div> </div><div> file.write(stringToPrintable(key.mKey)); // Key</div><div> </div><div>- if (currentEntry.bNLS) {</div><div>+ if (currentEntry.bNLS && locale != "C") {
</div><div> file.putChar('[');</div><div> file.write(locale); // Locale tag</div><div> file.putChar(']');</div><div>@@ -298,29 +279,37 @@</div><div> {</div><div> Q_ASSERT(!filePath().isEmpty());
</div><div> </div><div>- KEntryMap tempMap = toMerge;</div><div>+ KEntryMap writeMap = toMerge;</div><div> bool bGlobal = options & WriteGlobal;</div><div> </div><div>- int mergeCount = toMerge.count();</div>
<div> const KEntryMapIterator end = entryMap.end();</div><div> for (KEntryMapIterator it=entryMap.begin(); it != end; ++it) {</div><div>- const KEntryKey& key = it.key();</div><div>- if (key.bDefault
) {</div><div>- tempMap[key] = *it; // store default values in tempMap</div><div>+ if (!it->bDirty) // not dirty, doesn't overwrite entry in writeMap</div><div> continue;</div><div>- }
</div><div>- if (mergeCount > 0 && !it->bDirty)</div><div>- continue;</div><div> </div><div>- // only write back dirty entries that have the same</div><div>- // "globality" as the file
</div><div>- if (it->bGlobal == bGlobal && it->bDirty) {</div><div>- tempMap[key] = *it;</div><div>+ const KEntryKey& key = it.key();</div><div>+</div><div>+ // only write back dirty entries that have the same "globality" as the file
</div><div>+ if (it->bGlobal == bGlobal) {</div><div>+ KEntryKey defaultKey = key;</div><div>+ defaultKey.bDefault = true;</div><div>+ if (it->bDeleted && !entryMap.contains(defaultKey))
</div><div>+ writeMap.removeEntry(key); // remove the deleted entry if there is no default</div><div>+ else</div><div>+ writeMap[key] = *it;</div><div>+</div><div> it->bDirty = false;
</div><div> }</div><div> }</div><div> </div><div>- // now tempMap should contain all entries to be written</div><div>+ if (writeMap.isEmpty()) {</div><div>+ QFile file(filePath());</div><div>+ if (
file.exists())</div><div>+ file.remove(); // delete the file since it's going to be empty anyways</div><div>+ return true;</div><div>+ }</div><div>+</div><div>+ // now writeMap should contain only entries to be written
</div><div> // so write it out to disk</div><div> </div><div> // check if file exists</div><div>@@ -352,8 +341,9 @@</div><div> if (createNew)</div><div> file.setPermissions(QFile::ReadUser|QFile::WriteUser);
</div><div> </div><div>- writeEntries(locale, file, tempMap);</div><div>+ writeEntries(locale, file, writeMap);</div><div> </div><div>+ // FIXME is this needed anymore?</div><div> if ( !file.size() && ((fileMode == -1) || (fileMode == 0600)) ) {
</div><div> // File is empty and doesn't have special permissions: delete it.</div><div> file.abort();</div><div>@@ -432,7 +422,6 @@</div><div> if (lockFile->lock() == KLockFile::LockStale) // attempt to break the lock
</div><div> lockFile->lock(KLockFile::ForceFlag);</div><div> return lockFile->isLocked();</div><div>- return true;</div><div> }</div><div> </div><div> void KConfigIniBackend::unlock()</div><div><br class="webkit-block-placeholder">
</div></div>