Always save view settings when switching from one view mode to another
ClosedPublic

Authored by meven on May 28 2019, 11:36 AM.

Details

Summary
  • Add zoom setings for each view kind
  • Save and restore zoom when view is changed
  • Add default icon size of 64 px for icons view and 32px for compact view

Diff Detail

Repository
R241 KIO
Branch
icon-save2
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12240
Build 12258: arc lint + arc unit
meven created this revision.May 28 2019, 11:36 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 28 2019, 11:36 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.May 28 2019, 11:36 AM
meven updated this revision to Diff 58779.May 28 2019, 11:43 AM

Make the expanded detail view and the regular one share the same zoom setting

meven updated this revision to Diff 58780.May 28 2019, 11:50 AM

Removing parent commit

Nice! What do you think about moving the if (configGroup && itemView) { into the new function and calling it something more conditional, like writeIconZoomSettingsIfNeeded? Also it seems like there could be some code shared in a new function so that KDirOperator::Private::iconSizeForViewType() and KDirOperator::Private::writeIconZoomSettings() don't have to have so much duplicated code. They're both in the same scope so they have access to the same set of variables which should hopefully make it possible.

meven updated this revision to Diff 58811.May 29 2019, 7:12 AM

Rename writeIconZoomSettings to writeIconZoomSettingsIfNeeded, some code improvements

meven updated this revision to Diff 58816.May 29 2019, 7:40 AM

Add a zoomSettingsForViewForView to group viewKind to zoom setting name and default value

meven added a comment.May 29 2019, 7:40 AM

Nice! What do you think about moving the if (configGroup && itemView) { into the new function and calling it something more conditional, like writeIconZoomSettingsIfNeeded? Also it seems like there could be some code shared in a new function so that KDirOperator::Private::iconSizeForViewType() and KDirOperator::Private::writeIconZoomSettings() don't have to have so much duplicated code. They're both in the same scope so they have access to the same set of variables which should hopefully make it possible.

I did your two nice suggestions. See writeIconZoomSettingsIfNeeded and zoomSettingsForViewForView.

meven added inline comments.May 29 2019, 2:13 PM
src/filewidgets/kdiroperator.cpp
2691

Perhaps we could use a more sensible default value for ColumnView.

What would you say about 32px ?

meven updated this revision to Diff 58854.May 29 2019, 3:24 PM

Rebasing

meven marked an inline comment as done.May 29 2019, 3:26 PM
ngraham added inline comments.May 29 2019, 6:10 PM
src/filewidgets/kdiroperator.cpp
2691

That sounds like a good default size.

meven marked an inline comment as done.May 31 2019, 6:58 AM
meven added a comment.Jun 2 2019, 8:00 PM

With D21315 landing, great time to have a look at this review ;)

Could you rebase this on current master? It doesn't apply for me right now.

meven updated this revision to Diff 59058.Jun 3 2019, 10:12 AM

Rebasing on master

meven added a comment.Jun 3 2019, 10:12 AM

Could you rebase this on current master? It doesn't apply for me right now.

Thanks, I forgot to do it ;) Should be fixed now.

ngraham accepted this revision.Jun 3 2019, 1:39 PM

If you're going to rename the functions because these actions no longer really toggle anything, I think we need to keep a verb in there. Suggest something like _k_slotShowDetailsView or _k_slotSwitchToDetailsView

Otherwise LGTM in terms of code and behavior!

This revision is now accepted and ready to land.Jun 3 2019, 1:39 PM
meven added a comment.Jun 3 2019, 6:01 PM

If you're going to rename the functions because these actions no longer really toggle anything, I think we need to keep a verb in there. Suggest something like _k_slotShowDetailsView or _k_slotSwitchToDetailsView

I was renaming those just to be consistent with the already existing similar functions _k_slotDetailedView() _k_slotSimpleView() _k_slotTreeView() and _k_slotDetailedTreeView.
I don't have a strong opinion about adding a verb or keeping as it is now.
Given the added information, I will follow recommendation.

meven edited the summary of this revision. (Show Details)Jun 3 2019, 6:01 PM
meven edited the summary of this revision. (Show Details)Jun 4 2019, 9:10 AM
meven added a comment.Jun 6 2019, 6:48 AM

Any feedback @ngraham ?

Your reasoning makes sense. Shipit! :)

meven edited the summary of this revision. (Show Details)Jun 6 2019, 5:40 PM
This revision was automatically updated to reflect the committed changes.