Fix a bug in KCM cursor theme, after applying change, cursor preview don't work
ClosedPublic

Authored by bport on Oct 9 2019, 2:37 PM.

Details

Summary

This fix work only after applying other patch (but fix an issue that exist on master) (24519)
Bug: When you go to the kcm cursor, if you go hover a theme the "real" cursor change to show a preview. After applying a new theme the preview don't work anymore before you change the cursor size.
In order to work we need a call to setTheme that is done by setCursorSize.
Currently the if (m_currentSize == size) prevent the call to setTheme, so I removed it and removed the unused signal

Diff Detail

Repository
R119 Plasma Desktop
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 17511
Build 17529: arc lint + arc unit
bport created this revision.Oct 9 2019, 2:37 PM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 9 2019, 2:37 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bport requested review of this revision.Oct 9 2019, 2:37 PM
davidedmundson requested changes to this revision.Oct 9 2019, 3:00 PM
davidedmundson added a subscriber: davidedmundson.

I don't understand this patch in relation to the title.
If no-one uses this property this shouldn't make a difference?

This revision now requires changes to proceed.Oct 9 2019, 3:00 PM
bport requested review of this revision.Oct 9 2019, 3:50 PM
bport edited the summary of this revision. (Show Details)
davidedmundson accepted this revision.Oct 9 2019, 3:53 PM
This revision is now accepted and ready to land.Oct 9 2019, 3:53 PM
ervin requested changes to this revision.Oct 9 2019, 8:34 PM

Overall this doesn't look like the proper fix to me. A setter should not do anything if we pass the current value to it. From your description it seems to indicate that a call to setTheme is missing in some situations, I think it'd lead to a better fix if the exact condition when this happens is determined and then the extra setTheme call inserted just then. Changing a setter semantic to get a side-effect looks wrong to me.

kcms/cursortheme/xcursor/previewwidget.h
34

I'd keep it as is for the reasons I mentioned in another patch: avoid write only properties, it's really annoying on the QML side.

This revision now requires changes to proceed.Oct 9 2019, 8:34 PM
bport updated this revision to Diff 67607.Oct 10 2019, 1:49 PM

a cleaner approach to fix the bug

bport marked an inline comment as done.Oct 10 2019, 1:54 PM
ervin accepted this revision.Oct 15 2019, 8:58 AM
This revision is now accepted and ready to land.Oct 15 2019, 8:58 AM
This revision was automatically updated to reflect the committed changes.