Dolphin-style view modes in the file dialog
ClosedPublic

Authored by meven on May 21 2019, 9:46 AM.

Details

Summary

Add Icons, Compact and Details view to the filewidgets.

Added an "Allow Expansion" menu option to toggle between Detailview and DetailTreeView
Moved KDirOperatiorIconView to its own class file.
Set default icon size for Icons view to 64px (piggy back D12326)
Add a zoom factor specific to compact view (aka simple view with left icons)

Based on P388 and D21283

Fixes "Make the view modes match the ones in Dolphin (e.g. Icons, Compact, and Details, with an option for "Allow expansion" in the settings menu" of T8552

BUG: 86838
FIXED-IN: 5.60

Test Plan

Manual

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.May 21 2019, 9:46 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 21 2019, 9:46 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.May 21 2019, 9:46 AM
meven edited the test plan for this revision. (Show Details)May 21 2019, 9:53 AM
ngraham edited the summary of this revision. (Show Details)May 21 2019, 1:26 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the summary of this revision. (Show Details)May 21 2019, 3:16 PM

This is fantastic. You fixed all the issues I was having, and the UX is practically perfect. The code looks totally sane too. In my testing I found only one small issue: when changing the icon size in one view and then switching to another view and then back, the first view did not save the new size; saving icon sizes only happens when closing the window. With more view modes prominently exposed now, it might be nice to save newicon sizes instantly, or when switching to a different view, rather than only saving when closing the dialog.

src/filewidgets/kdiroperator.cpp
2022

Now I wonder if this should say "Allow expansion in Details view" and not disable itself when using a different view. Otherwise it might be hard for people to figure out what it does or how to activate it.

src/filewidgets/kdiroperator.h
963

Unnecessary whitespace change

meven updated this revision to Diff 58424.May 21 2019, 5:22 PM

Reword Allow Expansion to Allow Expansion in Details View

meven marked 2 inline comments as done.May 21 2019, 5:23 PM
meven updated this revision to Diff 58642.May 25 2019, 12:30 PM

Fix missing ')'

meven added a comment.Tue, May 28, 7:18 AM
This comment was removed by meven.
meven added a reviewer: VDG.Tue, May 28, 9:18 AM
anthonyfieroni added inline comments.
src/filewidgets/kdiroperator.cpp
1955

Can we make them reusable, it should be in framework and then accessed in Dolphin.

meven updated this revision to Diff 58774.Tue, May 28, 11:02 AM

Allow to save different zoom settings for all view kinds

meven updated this revision to Diff 58775.Tue, May 28, 11:09 AM

Remove icon zoom settings change this belong to another review

meven updated this revision to Diff 58776.Tue, May 28, 11:12 AM

Bad comment

meven marked an inline comment as done.Tue, May 28, 12:22 PM
meven added inline comments.
src/filewidgets/kdiroperator.cpp
1955

Well there are pretty much reusable already, their actions are available in the actionCollection, although not documented.

But reusing kdiroperator in dolphin is another matter.
The views here are simply based on existing kdiroperator views and most of their bugs and features gaps with Dolphin remains.

ngraham accepted this revision.Tue, May 28, 3:20 PM

Works perfectly and the code looks sensible!

src/filewidgets/kdiroperator.cpp
1955

Yeah, I'm afraid it isn't possible without putting KDirOperator back into Dolphin, which was removed on purpose many years ago. I like to see Dolphin once again use a re-usable component too, but that's outside the scope of this patch.

This revision is now accepted and ready to land.Tue, May 28, 3:20 PM
meven updated this revision to Diff 58832.Wed, May 29, 11:22 AM
meven marked an inline comment as done.

Avoid using Toggle in view activation callback names

I have made a small code change.

Should I wait for some more review before landing this ?

We're a bit close to Frameworks 5.59 tagging (June 1st). For large patches like this, sometimes it's worth waiting for the next release to give it a month of testing rather than rushing it in close to the deadline. I say let's wait until after tagging to land it, or else if you get a thumbs-up review from anyone else before June 1st then we can get it in for 5.59. Does that sound sane and sensible?

meven added a comment.EditedWed, May 29, 6:36 PM

We're a bit close to Frameworks 5.59 tagging (June 1st). For large patches like this, sometimes it's worth waiting for the next release to give it a month of testing rather than rushing it in close to the deadline. I say let's wait until after tagging to land it, or else if you get a thumbs-up review from anyone else before June 1st then we can get it in for 5.59. Does that sound sane and sensible?

Good suggestion, I keep forgetting about the monthly release and the opportunity of beginning of months to get things merged.

5.59 has been tagged, so I think this can land now.

meven edited the summary of this revision. (Show Details)Sun, Jun 2, 7:57 PM
This revision was automatically updated to reflect the committed changes.