D16852: Add Documents to the default list of Places

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:1208a51e9abf: Add Documents to the default list of Places 
(authored by acrouthamel, committed by ngraham).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16852?vs=45388=58385

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

AFFECTED FILES
  src/filewidgets/kfileplacesmodel.cpp

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

D16852: Add Documents to the default list of Places

[Nathaniel Graham]

ngraham added a comment.


  With the addition of the Tags section, there's already a scrollbar, so 
there's no harm in doing this.

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

D16852: Add Documents to the default list of Places

[Andrew Crouthamel]

acrouthamel added a comment.


  In D16852#362344 , @ngraham wrote:
  
  > 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

D16852: Add Documents to the default list of Places

[Nathaniel Graham]

ngraham added a comment.


  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.

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

D16852: Add Documents to the default list of Places

[Andrew Crouthamel]

acrouthamel added a comment.


  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.

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

D16852: Add Documents to the default list of Places

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

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

D16852: Add Documents to the default list of Places

[Andrew Crouthamel]

acrouthamel added a comment.


  In D16852#360940 , 
@elvisangelaccio wrote:
  
  > @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!

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

D16852: Add Documents to the default list of Places

[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

D16852: Add Documents to the default list of Places

[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

D16852: Add Documents to the default list of Places

[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
   

D16852: Add Documents to the default list of Places

[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

D16852: Add Documents to the default list of Places

[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