kcm/fonts: set initial settings correctly
ClosedPublic

Authored by bshah on Nov 4 2019, 10:30 AM.

Details

Summary

During initial load we check if certain settings are set or not, and if
they are not set, we set it to rgb/slight (recommended settings by
Plasma team).

however state management of this is broken, previous flow was,

  • try to get configuration
  • if configuration is not available, set state to desired settings
  • set original state to current state value(!)
  • write configuration to the file once complete state is written (all other settings)

This is fine, when setting is changing from something other then none or
not-set. But if it is not-set, both original state and current state
ended up being desired settings, which write function would happily
ignore since it thinks config never changed.

So, if we encounter not-set configuration, preserve that as a original
setting to get configs written correctly.

This patch on it's own have no visible effect, kcm is still broken due
to lack of kcminit which writes correct configuration at startup, but
this is one of obvious bugfix for follow-up patch series.

Test Plan

with upcoming patch to add kcminit, it correctly applies rgb/slight

Diff Detail

Repository
R119 Plasma Desktop
Branch
bshah/fonts
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18486
Build 18504: arc lint + arc unit
bshah created this revision.Nov 4 2019, 10:30 AM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 4 2019, 10:30 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bshah requested review of this revision.Nov 4 2019, 10:30 AM
fvogt added a comment.Nov 4 2019, 10:39 AM

That seems like just the order of assignments is wrong and more logic isn't actually needed. What about something like this:

KXftConfig::SubPixel::Type spType = KXftConfig::SubPixel::NotSet;
// we get subpixel type from config
xft.getSubPixelType(spType);
m_originalState.subPixel = spType;
if(spType == KXftConfig::SubPixel::NotSet) {
    spType = KXftConfig::SubPixel::Rgb;
}

setSubPixel(spType);
bshah added a comment.Nov 4 2019, 10:42 AM

That seems like just the order of assignments is wrong and more logic isn't actually needed. What about something like this:

Important thing to note is, setSubPixelType doesn't actually write config. It just sets the internal configuration. Actual setting of value is done in FontAASettings::save.

bshah added a comment.Nov 4 2019, 10:44 AM

OH wait! nvm, I see what you mean..

bshah updated this revision to Diff 69258.Nov 4 2019, 10:54 AM

update based on fvogt comment

fvogt accepted this revision.Nov 4 2019, 4:17 PM

LGTM. This now ignores the return value of the xft.get* calls, but that seems to be useless anyway:

bool KXftConfig::getSubPixelType(SubPixel::Type &type)
{
    type = m_subPixel.type;
    return SubPixel::None != m_subPixel.type;
}
This revision is now accepted and ready to land.Nov 4 2019, 4:17 PM
This revision was automatically updated to reflect the committed changes.