D27785: [Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary
ahmadsamir updated this revision to Diff 77268. ahmadsamir retitled this revision from "[Fonts KCM] Change how nearestExistingFonts() finds a matching font" to "[Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary". ahmadsamir edited the summary of this revision. ahmadsamir edited the test plan for this revision. ahmadsamir added a subscriber: bport. ahmadsamir added a comment. Tweak commit message REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27785?vs=77265&id=77268 BRANCH arcpatch-D27808 (branched from master) REVISION DETAIL https://phabricator.kde.org/D27785 AFFECTED FILES kcms/fonts/fonts.cpp kcms/fonts/fonts.h To: ahmadsamir, #plasma, davidedmundson, broulik, ervin, meven Cc: bport, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27785: [Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary
ahmadsamir marked 3 inline comments as done.
ahmadsamir added inline comments.
INLINE COMMENTS
> bport wrote in fonts.cpp:115
> You can use && to have only one if there
OK.
> bport wrote in fonts.cpp:552
> Those change will compare font twice, here and on setters so I will keep old
> code there
Indeed; looking at the code generated by kconfig compiler in
/kcms/fonts/fontsettings.h:
/**
Set General font
*/
void setFont( const QFont & v )
{
if (v != mFont && !isFontImmutable()) {
mFont = v;
Q_EMIT fontChanged();
}
}
I will revert. And another point is that the code in fonts.cpp doesn't need to
check for immutability since it's checked by the setters.
> bport wrote in fonts.cpp:576
> I will keep that until we have a proper tested fix for
> https://phabricator.kde.org/D27452
> can prevent bug and ensure apply button is on the good state on all case
OK, fair point, I will revert that bit.
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D27785
To: ahmadsamir, #plasma, davidedmundson, broulik, ervin, meven
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus,
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27785: [Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary
ahmadsamir updated this revision to Diff 77258. ahmadsamir edited the summary of this revision. ahmadsamir edited the test plan for this revision. ahmadsamir removed a reviewer: bport. ahmadsamir added a comment. More detailed commit message REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27785?vs=76868&id=77258 BRANCH l-qfontdlg-track-nearest (branched from master) REVISION DETAIL https://phabricator.kde.org/D27785 AFFECTED FILES kcms/fonts/fonts.cpp kcms/fonts/fonts.h To: ahmadsamir, #plasma, davidedmundson, broulik, ervin, meven, bport Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27785: [Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary
bport requested changes to this revision.
bport added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> fonts.cpp:115
> +if (!sameFont) {
> +if (font.weight() == QFont::Normal && font.styleName().isEmpty()
> +&& result.weight() == QFont::Normal
You can use && to have only one if there
> fonts.cpp:552
> {
> -m_settings->setFont(nearestExistingFont(m_settings->font()));
> -m_settings->setFixed(nearestExistingFont(m_settings->fixed()));
Those change will compare font twice, here and on setters so I will keep old
code there
> fonts.cpp:576
> -// KCM expect save state to be false at this point (can be true because
> of setNearestExistingFonts
> -setNeedsSave(false);
> }
I will keep that until we have a proper tested fix for
https://phabricator.kde.org/D27452
can prevent bug and ensure apply button is on the good state on all case
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D27785
To: ahmadsamir, #plasma, davidedmundson, broulik, ervin, meven, bport
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus,
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27785: [Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary
ahmadsamir added a comment. FTR, removing setNeedsSave(false) from load(), it seems to work sometimes, and then some other times it doesn't work (i.e. changing anything, Apply doesn't get enabled)... REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D27785 To: ahmadsamir, #plasma, davidedmundson, broulik, ervin, meven Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27785: [Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary
ahmadsamir updated this revision to Diff 76868. ahmadsamir added reviewers: broulik, ervin, meven. ahmadsamir added a comment. Tweak REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27785?vs=76779&id=76868 BRANCH l-fonts-kcm-nearest (branched from master) REVISION DETAIL https://phabricator.kde.org/D27785 AFFECTED FILES kcms/fonts/fonts.cpp To: ahmadsamir, #plasma, davidedmundson, broulik, ervin, meven Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27785: [Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary
ahmadsamir added a comment. TODO: fix the font dialog to select the correct "Regular"-like style (I have a working patch but needs more work). REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D27785 To: ahmadsamir, #plasma, davidedmundson Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27785: [Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary
ahmadsamir updated this revision to Diff 76779. ahmadsamir edited the summary of this revision. ahmadsamir added a comment. Remove setNeedsSave(false) from load(), doesn't seem to be needed any more REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27785?vs=76778&id=76779 BRANCH l-fonts-kcm-nearest (branched from master) REVISION DETAIL https://phabricator.kde.org/D27785 AFFECTED FILES kcms/fonts/fonts.cpp To: ahmadsamir, #plasma, davidedmundson Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27785: [Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary
ahmadsamir created this revision. ahmadsamir added reviewers: Plasma, davidedmundson. Herald added a project: Plasma. ahmadsamir requested review of this revision. REVISION SUMMARY A "Regular" font will have an empty styleName e.g. "DejaVu Sans,12,-1,5,50,0,0,0,0,0" so that setBold(true) can work; we deliberately clear the styleName for "Regular"-like font styles when saving via KConfig, see: writeEntryGui() from kconfiggroupgui.cpp and https://phabricator.kde.org/D27735 for more gory details. Change nearestExistingFont() to return the same font it's been called on if the search result gives us the same font (same weight, same style), and interpret the case when the styleName is empty to mean it's using a "Regular"-like style if the font weight is QFont::Normal and the styleName is "Regular|Normal|Book|Roman". Change setNearestExistingFonts() to set the fonts only if nearestExistingFont() returns a different font. TEST PLAN - Remove ",Regular|Normal|Book|Roman" from *font* entries in kdeglobals - Open the fonts kcm, notice that the Apply button is enabled even when you haven't changed anything - Apply the diff then try again, the Apply button should be disabled until you actually change something REPOSITORY R119 Plasma Desktop BRANCH l-fonts-kcm-nearest (branched from master) REVISION DETAIL https://phabricator.kde.org/D27785 AFFECTED FILES kcms/fonts/fonts.cpp To: ahmadsamir, #plasma, davidedmundson Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
