[KCM Fonts] force need save to false during load to avoid state to be true too early
ClosedPublic

Authored by bport on Thu, Feb 13, 8:17 PM.

Details

Summary

This will resolve a bug (apply never enabled). Bug occurs (at least) when kdeglobals contains QFont serialization without styleName (old style)

BUG: 416358
FIXED-IN: 5.18.1

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bport created this revision.Thu, Feb 13, 8:17 PM
Restricted Application added a project: Plasma. · View Herald TranscriptThu, Feb 13, 8:17 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bport requested review of this revision.Thu, Feb 13, 8:17 PM

Started building plasma-desktop over here on an affected machine with this diff applied

The-Feren-OS-Dev accepted this revision.EditedThu, Feb 13, 9:08 PM

I can confirm this fixes the bug - apply now becomes sensitive when changing a setting in Fonts

This revision is now accepted and ready to land.Thu, Feb 13, 9:08 PM

Other people are confirming that this patch also fixes the issue:
https://bugs.kde.org/show_bug.cgi?id=416358

I was not able to reproduce the issue, but this patch causes no regressions for me.

I still don't fully understand the bug and the fix

So what we're saying is:
Something changes us to needs save early on startup
We emit changed early
That gets lost (?)
So we have to reset back to unchanged after loading so that future changes will enable the apply button?

I still don't fully understand the bug and the fix

So what we're saying is:
Something changes us to needs save early on startup
We emit changed early
That gets lost (?)
So we have to reset back to unchanged after loading so that future changes will enable the apply button?

In case it makes any more sense: The bug made Fonts KCM never indicate that unsaved changes have been made, meaning you can never Apply any changes since it doesn't think anything was changed that is now pending being applied. Apply is always insensitive. This patch however fixes that for me and others who were affected by this issue.

This patch however fixes that for me

Sure, but why/how?

@davidedmundson
We have 2 bugs :

  • KCModuleQML apply button will stay disabled forever if at the end of load function need save is true (probably qml connection not yet done or something like that I guess, not yet found why) => We need to investigate on it too, I reproduced the same behavior with KCM icons by changing a setting at the end of load method.
  • In theory at the end of load we don't need to save data. However in some case for the font KCM data are dirty and need save. Came from Qt font comparison, if in your kdeglobals you have a font definition without style name, our algorithm to match a nearest font will add one, and font comparison between referential data and current data will return false because they are different.
  • My first approach was to fix algorithm to find the nearest font and keep style name empty if we don't have information. After this fix apply button work as expected, but when you adjust this font the selected style will be the first one on the list and not the one expected (regular)
  • Second approach by setting need save to false at the end of load allow us to be on the good state and the next change will allow us to enable apply button. By the way apply button will be enable directly because settings changed is trigered by another time after loading not sure where (didn't investigated yet)

About KCModuleQML didn't investigate a lot but KCModule (parent class) emit a changed false on showEvent (the way the handle to set need save to false. On qml side we use directly stNeedsSave (you can look at save method implementation), so a proper fix can be to add "d->configModule->setNeedsSave(false);" after loading

About KCModuleQML didn't investigate a lot but KCModule (parent class) emit a changed false on showEvent (the way the handle to set need save to false.

Aha! What a weird bit of code.

so I think I see what happens:

  1. KCModule::showEvent()
  2. this queues up a load and queues up a KCModule::changed(false)
  1. during load ConfigModule::setNeedsSave(true) is called we set d->_needsSave to true
  2. we emit ConfigModule::changed(true) which we proxy through to KCModule::changed(true)
  1. we then process the queued KCModule::setChanged(false) from the earlier KCModule::showEvent
  2. so we disable the button
  1. any subsequent changes in the KCM will call ConfigModule::setNeedsSave(true)

but this now matches d->_needsSave so it no-ops. Even though KCModule is out of sync.

Systemsettings only knows what KCModule signals, not ConfigModule.

This patch resets ConfigModule::d->_needsSave after step 3


In the morning can you confirm that commenting out the return in

void ConfigModule::setNeedsSave(bool needs)
{
    if (needs == d->_needsSave) {
        return;
    }

also fixes it.

If so, I understand it all, and will happily approve this patch.

bport added a comment.Fri, Feb 14, 7:45 AM

Yes emiting signal from setNeedsSave in all case fix stuff too

So this patch is here to solve for "short term"

In more long term we need to fix ConfigModule do you think is better to do a setNeedsSave(false) after loading or emiting signal in all case ?

meven accepted this revision.Fri, Feb 14, 8:06 AM

I can confirm the patch fix the issue.

Tested with

[General]
font=Noto Sans,10,-1,5,50,0,0,0,0,0

in .config/kdeglobals

With and without the patch, and the apply button now is highlighted when it the font setting was changed and the setting is saved and applied on Apply

meven edited the summary of this revision. (Show Details)Fri, Feb 14, 8:06 AM
davidedmundson accepted this revision.Fri, Feb 14, 8:11 AM

In more long term we need to fix ConfigModule do you think is better to do a setNeedsSave(false) after loading or emiting signal in all case ?

I think we need something, I've seen some similar report in the nightmode KCM about the apply button.

I would personally say the right thing is to remove this line:

QMetaObject::invokeMethod(this, "changed", Qt::QueuedConnection, Q_ARG(bool, false));

As I don't see what it is trying to do

bport added a comment.Fri, Feb 14, 8:36 AM

I will look at a proper fix in the next days

crossi accepted this revision.Fri, Feb 14, 8:37 AM

I can confirm this fix the issue here.

This revision was automatically updated to reflect the committed changes.