D12591: KFileWidget: Provide faster access to the icon position setting

[Henrik Fehlauer]

rkflx abandoned this revision.
Herald added a subscriber: kde-frameworks-devel.

REPOSITORY
  R241 KIO

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

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

D12591: KFileWidget: Provide faster access to the icon position setting

[Nathaniel Graham]

ngraham added a comment.


  In D12591#256212 , @rkflx wrote:
  
  > In D12591#255639 , 
@elvisangelaccio wrote:
  >
  > > > While the submenu itself is not advertised in the API docs, it was 
publicly accessible
  > >
  > > Where? Isn't it a member of `KDirOperator::Private`? (I'm referring to 
`decorationMenu`)
  >
  >
  > That was my first thought too, but unfortunately it leaks out via the 
`actionCollection()`. See my patch to K3b (D12598 
), where it was used that way. I'd rather 
not break third-party users I don't know about…
  >
  >  ---
  >
  > In D12591#255732 , @ngraham 
wrote:
  >
  > > One thought: with this patch, there will be two disabled and inapplicable 
items in the menu when you're not using Short View, which might confuse people. 
Might be appropriate to only show them for Short View.
  >
  >
  > Isn't not confusing people what the "disabled state" is there for, compared 
to making widgets invisible? At least that's how it's done in most other 
places: Disable options which don't apply. Granted, the connection to Short 
View is a bit hard to discover, but that was the case before the patch too 
(even more so).
  >
  > Nevertheless, I'll hold of with the patch for now. Another more radical way 
forward would be to merge Next and Above as two separate View modes like in 
Dolphin (and get rid of the rest?). Seems like we need more discussion about 
the modes in general, let's do that in T8552 
.
  
  
  +1, would approve. I've commented there.

REPOSITORY
  R241 KIO

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

To: rkflx, #frameworks, ngraham
Cc: ngraham, elvisangelaccio, michaelh, bruns

D12591: KFileWidget: Provide faster access to the icon position setting

[Henrik Fehlauer]

rkflx planned changes to this revision.
rkflx added a comment.


  In D12591#255639 , 
@elvisangelaccio wrote:
  
  > > While the submenu itself is not advertised in the API docs, it was 
publicly accessible
  >
  > Where? Isn't it a member of `KDirOperator::Private`? (I'm referring to 
`decorationMenu`)
  
  
  That was my first thought too, but unfortunately it leaks out via the 
`actionCollection()`. See my patch to K3b (D12598 
), where it was used that way. I'd rather 
not break third-party users I don't know about…
  
  ---
  
  In D12591#255732 , @ngraham wrote:
  
  > One thought: with this patch, there will be two disabled and inapplicable 
items in the menu when you're not using Short View, which might confuse people. 
Might be appropriate to only show them for Short View.
  
  
  Isn't not confusing people what the "disabled state" is there for, compared 
to making widgets invisible? At least that's how it's done in most other 
places: Disable options which don't apply. Granted, the connection to Short 
View is a bit hard to discover, but that was the case before the patch too 
(even more so).
  
  Nevertheless, I'll hold of with the patch for now. Another more radical way 
forward would be to merge Next and Above as two separate View modes like in 
Dolphin (and get rid of the rest?). Seems like we need more discussion about 
the modes in general, let's do that in T8552 
.

REPOSITORY
  R241 KIO

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

To: rkflx, #frameworks, ngraham
Cc: ngraham, elvisangelaccio, michaelh, bruns

D12591: KFileWidget: Provide faster access to the icon position setting

[Nathaniel Graham]

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


  +1, much better than having a submenu with only two items when the main menu 
is itself not very big. Works great.
  
  One thought: with this patch, there will be two disabled and inapplicable 
items in the menu when you're not using Short View, which might confuse people. 
Might be appropriate to only show them for Short View.

REPOSITORY
  R241 KIO

BRANCH
  icon-position-usability (branched from master)

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

To: rkflx, #frameworks, ngraham
Cc: ngraham, elvisangelaccio, michaelh, bruns

D12591: KFileWidget: Provide faster access to the icon position setting

[Elvis Angelaccio]

elvisangelaccio added a comment.


  > While the submenu itself is not advertised in the API docs, it was publicly 
accessible
  
  Where? Isn't it a member of `KDirOperator::Private`? (I'm referring to 
`decorationMenu`)

REPOSITORY
  R241 KIO

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

To: rkflx, #frameworks
Cc: elvisangelaccio, michaelh, bruns

D12591: KFileWidget: Provide faster access to the icon position setting

[Henrik Fehlauer]

rkflx added a dependent revision: D12598: FileView: Provide faster access to 
the icon position setting.

REPOSITORY
  R241 KIO

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

To: rkflx, #frameworks
Cc: michaelh, bruns

D12591: KFileWidget: Provide faster access to the icon position setting

[Henrik Fehlauer]

rkflx added a comment.


  If this gets approved, I'll do the same change to K3b for consistency, which 
was the only other app showing the submenu I could find on https://lxr.kde.org.

REPOSITORY
  R241 KIO

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

To: rkflx, #frameworks
Cc: michaelh, bruns

D12591: KFileWidget: Provide faster access to the icon position setting

[Henrik Fehlauer]

rkflx created this revision.
rkflx added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
rkflx requested review of this revision.

REVISION SUMMARY
  The file dialog allows to move the file name labels from next to the
  icon to right under the icon for Short View. However, having to go in
  a deeply nested Settings button > Icon Position > Above File Name
  submenu makes it both hard to discover and cumbersome to use. Ideally
  this setting could be accessed directly from the menu for good usability.
  
  Making it easy to change back settings is especially important in light
  of D12326 , which will enable Icons Above 
File Name by default.
  
  We linearize the submenu entries from the submenu to the top-level menu,
  which works just fine as there are only two entries, increasing the
  overall menu size by only one item while removing an entire submenu.
  
  While the submenu itself is not advertised in the API docs, it was
  publicly accessible, and therefore is kept for compatiblity reasons
  (although with a slight change in wording).
  
  Ref T8552 
  
  FIXED-IN: 5.47

TEST PLAN
  `kdialog --getopenfilename`, switch view modes via the settings
  button, check that the icon position settings are only enabled for
  View > Short View and still work properly.
  
  Before:
  F5826732: KIO-icon-position-before.png 
  
  After:
  F5826733: KIO-icon-position-after.png 

REPOSITORY
  R241 KIO

BRANCH
  icon-position-usability (branched from master)

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

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks
Cc: michaelh, bruns