D16852: Add Documents to the default list of Places

[Andrew Crouthamel]

acrouthamel created this revision.
acrouthamel added reviewers: Frameworks, Dolphin.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
acrouthamel requested review of this revision.

REVISION SUMMARY
This adds Documents to the default list of Places, as discussed with
general favor in D10245 <https://phabricator.kde.org/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.

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

AFFECTED FILES
src/filewidgets/kfileplacesmodel.cpp

To: acrouthamel, #frameworks, #dolphin
Cc: kde-frameworks-devel, ngraham, michaelh, bruns

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


+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.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: kde-frameworks-devel, ngraham, michaelh, bruns

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a subscriber: elvisangelaccio.
ngraham added a comment.
This revision is now accepted and ready to land.


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.

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: elvisangelaccio, kde-frameworks-devel, ngraham, michaelh, bruns

[Andrew Crouthamel]

acrouthamel added a comment.


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

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: elvisangelaccio, kde-frameworks-devel, ngraham, michaelh, bruns

[David C]

davidc added a comment.


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

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: davidc, elvisangelaccio, kde-frameworks-devel, ngraham, michaelh, bruns

[Nathaniel Graham]

ngraham added a task: T8349: Improve Places panel usability and presentation.

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: davidc, elvisangelaccio, kde-frameworks-devel, ngraham, michaelh, bruns

[Andrew Crouthamel]

acrouthamel added a comment.


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)]

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: davidc, elvisangelaccio, kde-frameworks-devel, ngraham, michaelh, bruns

[Elvis Angelaccio]

elvisangelaccio added a comment.


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

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: davidc, elvisangelaccio, kde-frameworks-devel, ngraham, michaelh, bruns

[Andrew Crouthamel]

acrouthamel added a dependent revision: D16967: Add Documents shortcut detection.

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: davidc, elvisangelaccio, kde-frameworks-devel, ngraham, michaelh, bruns

[Andrew Crouthamel]

acrouthamel added a comment.

Post by Elvis Angelaccio
@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 <https://phabricator.kde.org/D16967>. Thanks!

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: davidc, elvisangelaccio, kde-frameworks-devel, ngraham, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.


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:
F6431560: Scrollbar.png <https://phabricator.kde.org/F6431560>

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 <https://phabricator.kde.org/T9795>.

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: davidc, elvisangelaccio, kde-frameworks-devel, ngraham, michaelh, bruns

[Andrew Crouthamel]

acrouthamel added a comment.


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

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: davidc, elvisangelaccio, kde-frameworks-devel, ngraham, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.


Well, the extra space gained from D15739 <https://phabricator.kde.org/D15739> would also be taken up by D7446 <https://phabricator.kde.org/D7446>. :)

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

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: davidc, elvisangelaccio, kde-frameworks-devel, ngraham, michaelh, bruns

[Andrew Crouthamel]

acrouthamel added a comment.

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

I'll poke around.

REPOSITORY
R241 KIO

BRANCH
add-documents (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D16852

To: acrouthamel, #frameworks, #dolphin, ngraham
Cc: davidc, elvisangelaccio, kde-frameworks-devel, ngraham, michaelh, bruns