Fix folderview popup mode list view icon size inheriting icon view icon size
ClosedPublic

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

Details

Summary

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.

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.

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.Mar 15 2020, 2:32 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 15 2020, 2:32 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
pereira.alex requested review of this revision.Mar 15 2020, 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.Mar 16 2020, 8:41 PM
This revision now requires changes to proceed.Mar 16 2020, 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)Mar 17 2020, 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 ?

Sorry for the delay, I will resume reviewing this soon.

Sorry for the delay, I will resume reviewing this soon.

No worries :) Thank you for your support !

ngraham requested changes to this revision.Apr 9 2020, 11:53 PM

I still don't think we need three values in the config file for only two view modes.

This revision now requires changes to proceed.Apr 9 2020, 11:53 PM

I still don't think we need three values in the config file for only two view modes.

well ... I can use only one value in config file, but then the user loses the previous selected size when he changes modes ( list to icon and back ).
also i could instead of 2 other values, use a "previous value of other mode", but that might not make much sense/readability for anyone else maybe ( and loses meaning if there ever is more than 2 modes ).

keeping modes selected sizes ( when switching modes ), this is the best i think. the rest of the code of the plasmoid, only looks at the default size value, and i think its best than filling the code with conditionals. also keeps backwards compability.

the only promise I can make is that i will be maintaining what i change, so if i can figure out a better way i improve it.

sorry if things aren't good enough, please believe that it isn't out of laziness or lack of interest.

I would give each view mode its own config value and adjust the code with conditionals; that's not a problem. There are no backwards compatibility concerns here if you do that, except for people currently using list view with gigantic icons, which will become small, fixing the bug. If they complain, we can just tell them that it was a bug and if they want huge list icons again, they'll have to configure them that way deliberately. :)

Alternatively we can give up on allowing the icon size in list view to be configured and just always use a small size.

pereira.alex added a comment.EditedApr 10 2020, 1:12 AM

I would give each view mode its own config value and adjust the code with conditionals; that's not a problem. There are no backwards compatibility concerns here if you do that, except for people currently using list view with gigantic icons, which will become small, fixing the bug. If they complain, we can just tell them that it was a bug and if they want huge list icons again, they'll have to configure them that way deliberately. :)

Alternatively we can give up on allowing the icon size in list view to be configured and just always use a small size.

Sorry, maybe i didn't explain it very well: that was like I had previously, but in that way, it would actually not fix the bug. the bug is that the rest of the plasmoid relies on a single size value. ( which I believe was the original bug report and what I wanted to fix ).

Maybe its just coding styles/schools, but I honestly think its better this way, update the size when configuring and then use default size everywhere, than adding bunch of conditions through out the code, being error prone.

but well, if it must be so... then ok

pereira.alex updated this revision to Diff 80557.EditedApr 19 2020, 4:20 PM

Trying to fix the bug:

So reverted the changes I did and focused on fixing the bug.
It was missing a connection to list view mode change.

So now one can change view mode, change icon size in icon view mode and revert back to list view mode and the icon size will be the correct one.

@ngraham did I do something wrong ? If i did it was not intentional.

I am hoping that at least the bug fix can get included in 5.19.

Re-reviewing this is on my to-do list. Sorry for the delay!

Re-reviewing this is on my to-do list. Sorry for the delay!

oh no worries, i know you are a busy man :) and really this isn't an important bug. I was just doubtfull if everything was ok since I kinda moved the "goal of the task" to bugfix only.

ngraham accepted this revision.May 5 2020, 6:43 PM

Bug is fixed, code change seems sane. Sorry for the long wait!

This revision is now accepted and ready to land.May 5 2020, 6:43 PM
ngraham retitled this revision from Fix/Allow folderview popup mode icon and list icon size to Fix folderview popup mode list view icon size inheriting icon view icon size.May 5 2020, 6:45 PM
ngraham edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.