Fix/Allow folderview popup mode icon and list icon size
Needs ReviewPublic

Authored by pereira.alex on Sun, Mar 15, 2:32 PM.

Details

Reviewers
ngraham
Group Reviewers
Plasma
VDG
Summary

As reported in bug #418269,
the icon size is only selectable in icon view mode.
but selecting a different icon size on icon view mode also changes
the size of the icons in list view mode.

It is needed a plasmashell restart for them to go back to small (default) size.

This creates a problem since the list icon size is hardcoded by default to small,
but it seems it is being changed on other places of the code.

Upon discussion, there seems to be nothing wrong with having a list mode with difference icon sizes.
( for example i like them a little then small, as to be easier to view and click ).

BUG: 418269
FIXED-IN: 5.19.0

Test Plan

I tested this on icon folderview and normal desktop folderview, changing icon sizes in iconview and listview
and restarting plasmashell.

Diff Detail

Repository
R119 Plasma Desktop
Branch
fix-folderview-popup-icon-list-size (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24392
Build 24410: arc lint + arc unit
pereira.alex created this revision.Sun, Mar 15, 2:32 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSun, Mar 15, 2:32 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
pereira.alex requested review of this revision.Sun, Mar 15, 2:32 PM
ngraham added inline comments.
containments/desktop/package/contents/ui/ConfigIcons.qml
247–248

Don't comment out code, just remove it (applies to the next few changes too)

containments/desktop/package/contents/ui/FolderView.qml
775–779

since makeIconSize() now does only one thing, we should probably just remove the function and instead just iconSize to FolderTools.iconSizeFromTheme(plasmoid.configuration.iconSize) on line 561.

ngraham requested changes to this revision.Mon, Mar 16, 8:41 PM
This revision now requires changes to proceed.Mon, Mar 16, 8:41 PM

Removed commented code

Removed commented code as requested.
Didn't removed the function, there are two connections to the makeIconSize function
on line 1064 and 1072.
I edited like this, if no problem. I have to learn a few things and test more about
removing the function as to not introduce some bug. ( this weekend if not a problem with you ) ?

Also this is going as an arc diff ( amend ). Should I have created an new revision through
arc diff --create ?

Thanks for the review !

Nope, you did everything right!

Since visible is true by default, you can just remove the visible: true lines entirely now. And oersonally I would still remove the makeIconSize() function and replace its invocations with the one line of code in that function, but it's your call.

Remove visible: true since its default

As requested, removed visible: true lines as requested previously.
I prefer to keep makeIconSize() ( its what I would do for me,
although would probably rename it to something along the lines of configIconSize ).

ngraham edited the summary of this revision. (Show Details)Tue, Mar 17, 1:07 AM
ngraham edited the test plan for this revision. (Show Details)

Thanks!

Now we have a new problem though: the default icon size for list view is synchronized with the default icon size for icon view, so it's now gigantic by default:

We need for the default value itself to be different for icon vs list view, while allowing each one to be independent. Probably the cleanest way to do this is to define a new value in the config file for the default list view icon size (could be called something like listViewIconSize) and then change makeIconSize() (good thing you kept it, lol) to return return FolderTools.iconSizeFromTheme(plasmoid.configuration.listViewIconSize) when it's in list view.

Does that make sense?

Thanks!

Now we have a new problem though: the default icon size for list view is synchronized with the default icon size for icon view, so it's now gigantic by default:

We need for the default value itself to be different for icon vs list view, while allowing each one to be independent. Probably the cleanest way to do this is to define a new value in the config file for the default list view icon size (could be called something like listViewIconSize) and then change makeIconSize() (good thing you kept it, lol) to return return FolderTools.iconSizeFromTheme(plasmoid.configuration.listViewIconSize) when it's in list view.

Does that make sense?

Yes, it does !
I will look into it, and do like you said, a new listViewIconSize default !

Using separate configs for list and icon mode

Like previously talked, added listViewIconSize
to the config xml. ( i also fix a little typoo, hope it doesn't make the commit invalid )

My solution was to duplicate the slider, one with id iconSize and another with id listViewIconSize.
Sorry if this is a bad solution, I have to pick up the qt qml books !

But it seems to be working as intended, with each mode having it size saved and applied accordingly.

Making a second slider is fine, but it's not actually necessary. Instead, you could make cfg_iconSize and cfg_listViewIconSize not be aliases, and instead bind the single slider's value to whichever one is appropriate, like so:

value: viewMode.currentIndex === 1 ? cfg_iconSize : cfg_listViewIconSize
containments/desktop/package/contents/config/main.xml
102

This label needs updating now, to differentiate it from listViewIconSize

I did the changes above like you said in the patch above.
I didn't applied it yet, because on testing that same patch, I discovered a bug. I reverted the changes ( after saving the work in the above patch ) and tested the current implementation and it also has the bug.

The bug is hard to catch, but its like this:
If one selects listview and changes size, all is well. One can choose iconview mode and apply, and then go back to listview mode and apply and all is well. Also can change listview mode icon size and all is still well.
But if one selects iconview mode, changes icon size of iconview mode, and apply, then listviewmode will have the icon view mode icon size until the plasmashell is restarted. after the plasmashell restart all is well again.

I still didn't understood why this is happening, i have been debugging the settings file plasma-org.kde.plasma.desktop-appletsrc and the settings seem to be properly written.

I really want to understand and fix what is happening before final work.

Fixing bug on changing icon size between list/grid view

So with this commit, I fixed the bug that was happening explained in previous commit.
Actually, this was what was happening that cause the opening of the bug report.

Now, one can change between list and grid view mode, change size on list mode, change size on grid mode
and everything will work as should be ( no need to restart plasmashell ).

So it seems the code is using iconSize property for icons, with the exception of FolderView.qml
that uses the makeIconSize function. Instead of creating functions/if's to test which mode is being used,
I created two config variables: listViewIconSize and gridViewIconSize. The config qml (ConfigIcons.qml) will use those
two variables and update the iconSize config variable, which will then be used by the plasmoid as the icon size.

So to summarize, instead of "detecting" if list view or grid view mode,
config will update iconSize with the proper size from listViewIconSize or gridViewIconSize.

If this logic of code is fine, in the next work I will merge the patch I did previously to only use
one slider, and then remove the makeIconSize function.

Abandoning the existing iconSize config value and adding a new one called gridViewIconSize means that everyone'e existing icon view size will be reset, since nothing will be reading from the iconSize values in the config files anymore. You have two options here:

  1. Don't abandon iconSize for the icon view; keep using it even though its name isn't perfectly descriptive anymore
  2. Create a kconf update script to migrate people's config files so that the existing iconSize values get transformed into gridViewIconSize See https://techbase.kde.org/Development/Tools/Using_kconf_update

Abandoning the existing iconSize config value and adding a new one called gridViewIconSize means that everyone'e existing icon view size will be reset, since nothing will be reading from the iconSize values in the config files anymore. You have two options here:

  1. Don't abandon iconSize for the icon view; keep using it even though its name isn't perfectly descriptive anymore
  2. Create a kconf update script to migrate people's config files so that the existing iconSize values get transformed into gridViewIconSize See https://techbase.kde.org/Development/Tools/Using_kconf_update

Thank you for your review @ngraham ,

I think there was some misunderstanding on my explanation: I am not abandoning iconSize. In fact, I am doing the opposite! I am making iconSize always the "true" value. What I am doing is saving the values of grid icon size and list icon size in different variables ( to save the slider configs ) and then applying the proper sizing to iconSize !

So instead of " if Grid or List -> iconSize or listIconSize" ... I do "always iconSize, when changing modes/size update iconSize with proper size".

Use single slider and sync values

Removed duplicated slider, to only use one slider that uses and updates the correct value.

Added an event when changing viewmode grid or list, to load the slider with the correct value.

If there isn't any issue with the code and behaviour, I think its ready:

  • adds feature to allow choosing size on list mode
  • fixes the bug reported in that the list mode icon size updates properly without needing a plasmashell restart

Also forgot to mention:

Tested with removing the new values from .config/plasma-org.kde.plasma.desktop-appletsrc :

  • If user doesn't go to properties and changes size or view mode, the old/current value is respected and used ( which might be a problem, if the previous iconSize is from gridview mode and set to huge, the user will face from size small to size huge on the list )
  • If user changes the size or view mode, the default values for the size are used on the slider.

I could do a check to see if the property iconSize of list mode is configured. if not, return the default ?

Deal with upgrading configuration

Added code to deal with old configuration:

  • created a check to see if the user is using list mode view and doesn't have listViewIconSize set. If it doesnt, then use small icon size ( which was old behaviour )
  • created code to set the slider to correct size when its used the first time for users upgrading. It does it by setting the slider value to the current value. When saving, it will then deal with saving into the correct value.

I haven't found any problems/bugs so far on my testing ( manually editing the applets rc plasma file to fake like if it was an old configuration ). The only issue I have with this, is that that check will need to run whenever the folderview is opened.
You think it is worth it ? or don't check it and let users handle migration ?