D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Nathaniel Graham]

ngraham abandoned this revision.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #vdg, #dolphin, elvisangelaccio, broulik
Cc: clel, Zren, elvisangelaccio, meven, ngraham, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ahmadsamir, bruns, vkrause

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Nathaniel Graham]

ngraham commandeered this revision.
ngraham added a reviewer: broulik.
ngraham added a comment.


  You later did this in 
https://invent.kde.org/system/dolphin/-/merge_requests/309. :)

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #vdg, #dolphin, elvisangelaccio, broulik
Cc: clel, Zren, elvisangelaccio, meven, ngraham, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ahmadsamir, bruns, vkrause

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Claudius Ellsel]

clel added a comment.


  Hopefully this can proceed soon, this seems like a pretty nice step towards 
better maintainability and consistency to me. Looks like all major work is 
already done and only some minor things need discussion or changes. @broulik do 
you have time to look at it again in the near future?

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, #dolphin, elvisangelaccio
Cc: clel, Zren, elvisangelaccio, meven, ngraham, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, bruns

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Nathaniel Graham]

ngraham added a task: T9795: Use Places Panel code from KIO instead of private 
implementation.

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, #dolphin, elvisangelaccio
Cc: elvisangelaccio, meven, ngraham, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, bruns

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Elvis Angelaccio]

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


  Haven't tried the dolphin branch yet, just a comment on the API for now.
  From a quick look the patch looks good though.

INLINE COMMENTS

> kfileplacesview.h:68
> + */
> +bool showAll() const;
> +

This seems to be the name of a method that makes all items visible, not the 
name of a bool getter.

I know we already have `setShowAll()`, but we can deprecate it and find a 
better name. How about `setAllPlacesVisible()` / `allPlacesVisible()` / 
`allPlacesVisibleChanged()` ?

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, #dolphin, elvisangelaccio
Cc: elvisangelaccio, meven, ngraham, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Nathaniel Graham]

ngraham added a subscriber: elvisangelaccio.
ngraham added a comment.


  #frameworks  folks, #dolphin 
 folks, and/or @elvisangelaccio, how 
is this looking? It would be really nice to be able to use the same backend 
code for the Places panel in Dolphin and the file dialogs again. :)

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, #dolphin
Cc: elvisangelaccio, meven, ngraham, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> kfileplacesview.h:157
>  Q_PRIVATE_SLOT(d, void adaptItemSize())
> -Q_PRIVATE_SLOT(d, void _k_placeClicked(const QModelIndex &))
> +Q_PRIVATE_SLOT(d, void _k_placeClicked(const QModelIndex &)) // TODO 
> remove
>  Q_PRIVATE_SLOT(d, void _k_placeEntered(const QModelIndex &))

For KF6 ?

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, #dolphin
Cc: meven, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Nathaniel Graham]

ngraham accepted this revision as: VDG.
ngraham added a comment.


  Yep, my branches weren't up to date. Works fine now!

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, #dolphin
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Kai Uwe Broulik]

broulik added a comment.


  F7831018: Screenshot_20191222_162523.png 

  Works fine here. Are both branches up to date and are you clicking an empty 
area?

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, #dolphin
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Nathaniel Graham]

ngraham added a comment.


  I'm using your Dolphin branch but I don't see it.

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, #dolphin
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Kai Uwe Broulik]

broulik added a comment.


  I added icon size context menu in dolphin

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, #dolphin
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Nathaniel Graham]

ngraham added a comment.


  One thing I notice has been lost is the ability to choose the size of the 
icons. Dolphin currently exposes this in the context menu for places panel 
headers. The File dialog does something different and automatically resizes the 
icons according to how much vertical space is available in the dialog. However 
with this patch and your Dolphin branch, neither one is available and the icon 
sizes appear to be immutable.
  
  It would be nice to unify the behavior between Dolphin and the file dialog 
with this patchset. We get bugs about them being different.

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, #dolphin
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Nathaniel Graham]

ngraham added a comment.


  Fantastic. UI-wise, I agree that adopting Dolphin's more narrow paddings 
between list elements makes the file dialog feel a bit crowded. This is also a 
(very) minor complaint I have about Dolphin. I wonder if it wouldn't make sense 
to keep the existing paddings between list items, or reduce them only a tiny 
bit.

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, #dolphin
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Kai Uwe Broulik]

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

REVISION SUMMARY
  This includes:
  
  - Rendering hidden items desaturated when in "show all" mode
  - A getter for "show all"
  - Signals for place activation/clicking
  - Signals for middle clicking a place (for e.g. open in a new tab)
  - Possibility to add custom actions to the context menu
  - Possibility to provide own storage teardown procedure
  - Changing view icon size is forwarded to delegates
  
  The delegates are also made somewhat more compact to match Dolphin and 
spacing inbetween sections increased.

TEST PLAN
  Try with branch `broulik/kfileplacesview` in Dolphin.
  
  - Can I remove a `Q_PRIVATE_SLOT`?
  
  - Currently when enabling "show all" and clicking a place, "show all" is 
deactivated again. Dolphin keeps this permanently. Should we change this?
  
  Dolphin (from left to right: current custom view, `KFilePlacesView` current 
look, `KFilePlacesView` more compact new look to match Dolphin)
  F7822829: Screenshot_20191217_162724.png 

  Open dialog (from left to right: current look, more compact look, imho looks 
a bit crowded in conjunction with the file dialog :/0
  F7822826: Screenshot_20191217_162838.png 


REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/filewidgets/kfileplacesview.cpp
  src/filewidgets/kfileplacesview.h
  src/filewidgets/kfileplacesview_p.h

To: broulik, #frameworks, #vdg, #dolphin
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns