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

[Benjamin Port]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:0c4da0774e43: [KCM Fonts] force need save to false during 
load to avoid state to be true too… (authored by bport).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27384?vs=75639=75668

REVISION DETAIL
  https://phabricator.kde.org/D27384

AFFECTED FILES
  kcms/fonts/fonts.cpp

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Cyril Rossi]

crossi accepted this revision.
crossi added a comment.


  I can confirm this fix the issue here.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Benjamin Port]

bport added a comment.


  I will look at a proper fix in the next days

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added a comment.


  > 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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Méven Car]

meven accepted this revision.
meven added a comment.


  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`

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Benjamin Port]

bport added a comment.


  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 ?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[David Edmundson]

davidedmundson added a comment.


  > 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)
  
  
  
  3. during load ConfigModule::setNeedsSave(true) is called we set 
d->_needsSave to true
  4. we emit ConfigModule::changed(true) which we proxy through to 
KCModule::changed(true)
  
  5. we then process the queued KCModule::setChanged(false) from the earlier 
KCModule::showEvent
  6. so we disable the button
  
  7. 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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Benjamin Port]

bport added a comment.


  @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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[David Edmundson]

davidedmundson added a comment.


  > This patch however fixes that for me
  
  Sure, but why/how?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


  In D27384#611135 , @davidedmundson 
wrote:
  
  > 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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[David Edmundson]

davidedmundson added a comment.


  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?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Dominic Hayes]

The-Feren-OS-Dev accepted this revision.
The-Feren-OS-Dev added a comment.
This revision is now accepted and ready to land.


  I can confirm this fixes the bug.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson, 
The-Feren-OS-Dev
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson
Cc: The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Benjamin Port]

bport created this revision.
bport added reviewers: Plasma, broulik, ervin, crossi, meven, ngraham, 
davidedmundson.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
bport requested review of this revision.

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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27384

AFFECTED FILES
  kcms/fonts/fonts.cpp

To: bport, #plasma, broulik, ervin, crossi, meven, ngraham, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart