Add Documents to the default list of Places
AcceptedPublic

Authored by acrouthamel on Nov 13 2018, 3:37 AM.

Details

Summary

This adds Documents to the default list of Places, as discussed with
general favor in D10245. This adds a common XDG directory that is often
used by users, making it easier and faster by default to access files.

Test Plan

Created a new user and observed and clicked on Documents, and the other
default places locations.

Diff Detail

Repository
R241 KIO
Branch
add-documents (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4886
Build 4904: arc lint + arc unit
acrouthamel created this revision.Nov 13 2018, 3:37 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 13 2018, 3:37 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
acrouthamel requested review of this revision.Nov 13 2018, 3:37 AM
ngraham requested changes to this revision.Nov 13 2018, 3:39 AM

+1 conceptually, but...

I take it you didn't run ctest. :) The kfileplacesmodel test will need adjustment. It may be painful. You may regret your life choices.

This revision now requires changes to proceed.Nov 13 2018, 3:39 AM
ngraham accepted this revision.Nov 13 2018, 4:05 AM
ngraham added a subscriber: elvisangelaccio.

Oh, this doesn't actually have any effect on the test since the item is only created conditionally, and the testing environment's homedir doesn't create the XDG dirs by default. So the tests still pass, whoo. Though it isn't actually being tested, but that's also happening for the other conditional XDG items. Not your fault, and that should be fixed elsewhere.

This does make Dolphin's placesitemmodeltest fail though, since it's smarter and by adding a new item the expected counts are now all off. Since apps have been branched, that could be fixed on master without the need for ifdefs.

Though I'm accepting this now, please wait for approval from some other people too, like @elvisangelaccio.

This revision is now accepted and ready to land.Nov 13 2018, 4:05 AM

Thanks, I spent some time looking through Elvis' commits to the Dolphin PlacesItemModelTest. I believe I added m_hasDocumentsFolder in the various locations properly, but I am still getting a failure in the test. Any tips are appreciated @elvisangelaccio. My attempt is at https://paste.kde.org/pey6davmw

davidc added a subscriber: davidc.

Just wanted to say thanks for this. Adding Documents is the first thing I do when I open Dolphin on a fresh install.

I checked out the test log and see the following, yet I honestly can't figure out why it is off by one, since any line referencing hasDocumentsFolder is a ++ line for the count. Do I need to run this test on a new user account or something?

FAIL!  : PlacesItemModelTest::testDeletePlace() Compared lists differ at index 2.
   Actual   (places): "/home/andrew/Desktop"
   Expected (urls): "/home/andrew/Documents"
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(411)]
FAIL!  : PlacesItemModelTest::testPlaceItem(Places - Home) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testPlaceItem(Baloo - Documents) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testPlaceItem(Baloo - Today) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testPlaceItem(Devices - Floppy) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testTearDownDevice() Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testDefaultViewProperties(Places - Home) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testDefaultViewProperties(Baloo - Documents) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testDefaultViewProperties(Places - Audio) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testDefaultViewProperties(Baloo - Today) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testDefaultViewProperties(Devices - Floppy) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testClear() Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testHideItem() Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testSystemItems() Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testEditBookmark() Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testEditAfterCreation() Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testEditMetadata() Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testRefresh() Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testIcons(Places - Home) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testIcons(Baloo - Documents) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testIcons(Baloo - Today) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testIcons(Devices - Floppy) Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testDragAndDrop() Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testHideDevices() Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testDuplicatedEntries() Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::renameAfterCreation() Compared values are not the same
   Actual   (m_model->count())    : 19
   Expected (m_expectedModelCount): 18
   Loc: [/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
elvisangelaccio added a comment.EditedNov 17 2018, 12:14 PM

@acrouthamel Please open another diff with your patch that updates the dolphin test, so I can easily try it out

@acrouthamel Please open another diff with your patch thats updates the dolphin test, so I can try easily try it out

Ok, I created D16967. Thanks!

So the only problem I have with this patch is the fact that is results in Dolphin having a scrollbar for its Places panel because the default size of the main window is now one item's worth of height too short:

Do you think you could also submit a Dolphin patch that makes the default size of the window a little bit taller too?

I know it's silly to be concerned about a scrollbar, but it would be nice not to have it with the default view, especially since we haven't yet fixed https://bugs.kde.org/show_bug.cgi?id=301758.

I looked into fixing that once but concludes that it was almost impossible with the current implementation, which synthesizes its own scrollview from scratch. I think fixing that will require T9795: Use Places Panel code from KIO instead of private implementation.

I had submitted this with the intention of being a follow-up to D15739, as that would clear a slot for this. I think right now, T9795 is outside my skill set. I'd be glad to make D15739 a parent to this and wait for that to land.

Well, the extra space gained from D15739 would also be taken up by D7446. :)

I wasn't suggesting that you fix T9795 (it's beyond me too!), but rather than you could tackle the patch to make Dolphin's default window size a tad taller.

you could tackle the patch to make Dolphin's default window size a tad taller.

I'll poke around.