D28057: Fix/Allow folderview popup mode icon and list icon size

ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. Bug is fixed, code change seems sane. Sorry for the long wait! REPOSITORY R119 Plasma Desktop BRANCH fix-folderview-popup-icon-list-size (branched from master) REVISION DETAIL

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex added a comment. In D28057#657036 , @ngraham wrote: > 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

D28057: Fix/Allow folderview popup mode icon and list icon size

ngraham added a comment. Re-reviewing this is on my to-do list. Sorry for the delay! REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D28057 To: pereira.alex, #plasma, #vdg, ngraham Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack,

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex added a comment. @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. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D28057 To: pereira.alex, #plasma, #vdg,

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex updated this revision to Diff 80557. pereira.alex added a comment. Trying to fix the bug: So reverted the changed 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

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex added a comment. In D28057#645164 , @ngraham wrote: > 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,

D28057: Fix/Allow folderview popup mode icon and list icon size

ngraham added a comment. 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,

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex added a comment. In D28057#645140 , @ngraham wrote: > 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

D28057: Fix/Allow folderview popup mode icon and list icon size

ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. I still don't think we need three values in the config file for only two view modes. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D28057 To:

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex added a comment. In D28057#639054 , @ngraham wrote: > Sorry for the delay, I will resume reviewing this soon. No worries :) Thank you for your support ! REPOSITORY R119 Plasma Desktop REVISION DETAIL

D28057: Fix/Allow folderview popup mode icon and list icon size

ngraham added a comment. Sorry for the delay, I will resume reviewing this soon. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D28057 To: pereira.alex, #plasma, #vdg, ngraham Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh,

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex updated this revision to Diff 78758. pereira.alex added a comment. 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

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex added a comment. 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,

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex updated this revision to Diff 78755. pereira.alex added a comment. 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

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex added a comment. In D28057#634650 , @ngraham wrote: > 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

D28057: Fix/Allow folderview popup mode icon and list icon size

ngraham added a comment. 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

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex updated this revision to Diff 78238. pereira.alex added a comment. 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

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex added a comment. F8182687: folderview_work.patch 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

D28057: Fix/Allow folderview popup mode icon and list icon size

ngraham added a comment. 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

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex updated this revision to Diff 77808. pereira.alex added a comment. 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

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex added a comment. In D28057#628853 , @ngraham wrote: > 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:

D28057: Fix/Allow folderview popup mode icon and list icon size

ngraham added a comment. 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: F8180048: Screenshot_20200316_190912.png We need

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex updated this revision to Diff 77791. pereira.alex added a comment. 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

D28057: Fix/Allow folderview popup mode icon and list icon size

ngraham added a comment. 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

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex updated this revision to Diff 77789. pereira.alex added a comment. 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.

D28057: Fix/Allow folderview popup mode icon and list icon size

ngraham added inline comments. INLINE COMMENTS > ConfigIcons.qml:229 > Layout.fillWidth: true > -visible: !isPopup || viewMode.currentIndex === 1 > +visible: true //!isPopup || viewMode.currentIndex === 1 > Don't comment out code, just remove it (applies

D28057: Fix/Allow folderview popup mode icon and list icon size

pereira.alex created this revision. pereira.alex added reviewers: Plasma, VDG. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. pereira.alex requested review of this revision. REVISION SUMMARY As reported in bug #418269, the icon size is only selectable in icon view