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.
Details
- Reviewers
ngraham - Group Reviewers
Frameworks Dolphin - Maniphest Tasks
- T8349: Improve Places panel usability and presentation
- Commits
- R241:1208a51e9abf: Add Documents to the default list of Places
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
+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.
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.
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
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)]
@acrouthamel Please open another diff with your patch that updates the dolphin test, so I can easily try it out
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.
With the addition of the Tags section, there's already a scrollbar, so there's no harm in doing this.