Don't try to restore invalid user places
ClosedPublic

Authored by jtamate on Jul 12 2018, 9:53 AM.

Details

Summary

Don't restore user places with invalid attribute href.
This avoids asking for empty protocol in KFilePlacesItem::groupType and the assert in KProtocolInfoFactory::findProtocol.

Test Plan

Don't assert asking for invalid protocols when creating the save dialog with an invalid bookmark (empty href) in ~/.local/share/user-places.xbel

<bookmark href="">
 <title>Project Folder</title>
 <info>
  <metadata owner="http://freedesktop.org">
   <bookmark:icon name="folder-favorites"/>
  </metadata>
  <metadata owner="http://www.kde.org">
   <OnlyInApp>kdenlive</OnlyInApp>
  </metadata>
 </info>
</bookmark>

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jtamate created this revision.Jul 12 2018, 9:53 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 12 2018, 9:53 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
jtamate requested review of this revision.Jul 12 2018, 9:53 AM
dfaure requested changes to this revision.Sep 17 2018, 5:30 PM

Shouldn't this skip the invalid bookmark rather than abort completely ?

This revision now requires changes to proceed.Sep 17 2018, 5:30 PM
jtamate updated this revision to Diff 41879.Sep 18 2018, 7:19 AM

Continue processing the XML with invalid urls.
And a trivial code deduplication, I couldn't resist.

dfaure accepted this revision.Sep 18 2018, 7:23 AM
This revision is now accepted and ready to land.Sep 18 2018, 7:23 AM
This revision was automatically updated to reflect the committed changes.
dfaure added a comment.Oct 7 2018, 4:27 PM

This commit breaks kfileplacesmodeltest.

$ bin/kfileplacesmodeltest testInitialState testInitialList testInternalBookmarksHaveIds

  • Start testing of KFilePlacesModelTest *****

Config: Using QtTest library 5.12.0, Qt 5.12.0 (x86_64-little_endian-lp64 shared (dynamic) debug build; by GCC 7.3.1 20180323 [gcc-7-branch revision 258812])
QDEBUG : KFilePlacesModelTest::initTestCase() 18:21:44.046 kfileplacesmodeltest(22234) ?[32mSolid::Backends::Fake::FakeManager::parseMachineFile?[0m void Solid::Backends::Fake::FakeManager::parseMachineFile() Parsing fake computer XML: "/d/kde/src/5/frameworks/kio/autotests/fakecomputer.xml"

PASS : KFilePlacesModelTest::initTestCase()
PASS : KFilePlacesModelTest::testInitialState()
PASS : KFilePlacesModelTest::testInitialList()
FAIL! : KFilePlacesModelTest::testInternalBookmarksHaveIds() '!ids.contains(id)' returned FALSE. (Duplicated ID found!)

Loc: [/d/kde/src/5/frameworks/kio/autotests/kfileplacesmodeltest.cpp(315)]

PASS : KFilePlacesModelTest::cleanupTestCase()
Totals: 4 passed, 1 failed, 0 skipped, 0 blacklisted, 75ms

  • Finished testing of KFilePlacesModelTest *****

I have debugged it a little bit but I don't really understand the overall logic.

Many bookmarks have a UDI (like /org/kde/solid/fakehw/fstab/thehost/solidpath) and no URL, and those are the ones that are duplicated now.

When this code is called again, those bookmarks are now skipped in the first iteration, and therefore added by the next foreach (the one which says "Add bookmarks for the remaining devices, they were previous unknown").

Maybe this skipping should only be done for bookmarks with an empty UDI?

dfaure added a comment.Oct 7 2018, 4:27 PM

More generally, please please run the unittests after changing a class. I hate that I currently am the one doing this, on the day of the release, which forces me to bugfix such things, or delay the release.