D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
This revision was automatically updated to reflect the committed changes. Closed by commit R119:d28337b6d379: KCM Fonts port anti aliasing part to KPropertySkeletonItem (authored by bport). REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27156?vs=75890=77274 REVISION DETAIL https://phabricator.kde.org/D27156 AFFECTED FILES kcms/fonts/CMakeLists.txt kcms/fonts/fonts.cpp kcms/fonts/fonts.h kcms/fonts/fontsaasettings.cpp kcms/fonts/fontsaasettings.h kcms/fonts/fontsaasettingsbase.kcfg kcms/fonts/fontsaasettingsbase.kcfgc kcms/fonts/kxftconfig.h kcms/fonts/package/contents/ui/main.qml To: bport, #plasma, ervin, crossi, meven Cc: usta, 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
meven added a comment. Seems ready to land REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D27156 To: bport, #plasma, ervin, crossi, meven Cc: usta, 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
bport updated this revision to Diff 75890. bport added a comment. fix build. KRDB can't use Kxftconfig directly so we can't avoid stuff in kdeglobals REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27156?vs=75844=75890 REVISION DETAIL https://phabricator.kde.org/D27156 AFFECTED FILES kcms/fonts/CMakeLists.txt kcms/fonts/fonts.cpp kcms/fonts/fonts.h kcms/fonts/fontsaasettings.cpp kcms/fonts/fontsaasettings.h kcms/fonts/fontsaasettingsbase.kcfg kcms/fonts/fontsaasettingsbase.kcfgc kcms/fonts/kxftconfig.h kcms/fonts/package/contents/ui/main.qml To: bport, #plasma, ervin, crossi, meven Cc: usta, 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
bport updated this revision to Diff 75844. bport added a comment. Add missing space REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27156?vs=75813=75844 REVISION DETAIL https://phabricator.kde.org/D27156 AFFECTED FILES kcms/fonts/CMakeLists.txt kcms/fonts/fontremovenantialiasingkeys.upd kcms/fonts/fonts.cpp kcms/fonts/fonts.h kcms/fonts/fontsaasettings.cpp kcms/fonts/fontsaasettings.h kcms/fonts/fontsaasettingsbase.kcfg kcms/fonts/fontsaasettingsbase.kcfgc kcms/fonts/kxftconfig.h kcms/fonts/package/contents/ui/main.qml kcms/krdb/krdb.cpp To: bport, #plasma, ervin, crossi, meven Cc: usta, 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
crossi added inline comments.
INLINE COMMENTS
> fontsaasettings.cpp:305
> +{
> +if(dpi() == newDPI) {
> +return;
add space between `if` and `(` please
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D27156
To: bport, #plasma, ervin, crossi, meven
Cc: usta, 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
ervin accepted this revision. ervin added a comment. This revision is now accepted and ready to land. Looks good to me now, maybe try to get more reviewers before pushing though. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D27156 To: bport, #plasma, ervin, crossi, meven Cc: usta, 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
bport updated this revision to Diff 75813. bport added a comment. Add kconfupdate script to remove unused keys REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27156?vs=75541=75813 REVISION DETAIL https://phabricator.kde.org/D27156 AFFECTED FILES kcms/fonts/CMakeLists.txt kcms/fonts/fontremovenantialiasingkeys.upd kcms/fonts/fonts.cpp kcms/fonts/fonts.h kcms/fonts/fontsaasettings.cpp kcms/fonts/fontsaasettings.h kcms/fonts/fontsaasettingsbase.kcfg kcms/fonts/fontsaasettingsbase.kcfgc kcms/fonts/kxftconfig.h kcms/fonts/package/contents/ui/main.qml kcms/krdb/krdb.cpp To: bport, #plasma, ervin, crossi, meven Cc: usta, 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> fonts.cpp:136
> +for (KXftConfig::Hint::Style s : {KXftConfig::Hint::None,
> KXftConfig::Hint::Slight, KXftConfig::Hint::Medium, KXftConfig::Hint::Full}) {
> +auto *item = new
> QStandardItem(KXftConfig::description((KXftConfig::Hint::Style)s));
> +m_hintingOptionsModel->appendRow(item);
This C cast is unnecessary. Also the * is merely redundant with auto (you can
keep it but it's not necessary).
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D27156
To: bport, #plasma, ervin, crossi, meven
Cc: usta, 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
bport updated this revision to Diff 75541. bport added a comment. Fix reset and default button for Anti Aliasing area (states were ok, but UI values were not updated after clicking them) depends on https://phabricator.kde.org/D27342 REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27156?vs=75465=75541 REVISION DETAIL https://phabricator.kde.org/D27156 AFFECTED FILES kcms/fonts/CMakeLists.txt kcms/fonts/fonts.cpp kcms/fonts/fonts.h kcms/fonts/fontsaasettings.cpp kcms/fonts/fontsaasettings.h kcms/fonts/fontsaasettingsbase.kcfg kcms/fonts/fontsaasettingsbase.kcfgc kcms/fonts/kxftconfig.h kcms/fonts/package/contents/ui/main.qml kcms/krdb/krdb.cpp To: bport, #plasma, ervin, crossi, meven Cc: usta, 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> fonts.cpp:131
> +for (int t = KXftConfig::SubPixel::None; t <=
> KXftConfig::SubPixel::Vbgr; ++t) {
> +QStandardItem *item = new
> QStandardItem(KXftConfig::description((KXftConfig::SubPixel::Type)t));
> +m_subPixelOptionsModel->appendRow(item);
Please no C cast, use static_cast instead.
Also was a good place to use auto (DRY and all that) ;-)
> fonts.cpp:136
> +for (int s = KXftConfig::Hint::None; s <= KXftConfig::Hint::Full; ++s) {
> +QStandardItem * item = new
> QStandardItem(KXftConfig::description((KXftConfig::Hint::Style)s));
> +m_hintingOptionsModel->appendRow(item);
ditto (and no space after *)
> fontsaasettings.cpp:70
> +FontAASettingsStore(FontsAASettings *parent = nullptr)
> +: QObject(parent)
> +, m_settings(parent)
Wrong indentation
> fontsaasettings.cpp:224
> +bool m_subPixelChanged;
> +int m_hinting;
> +bool m_hintingChanged;
I'd go for the enums all the way until here of course
> fontsaasettings.h:35
> +Q_PROPERTY(int dpi READ dpi WRITE setDpi NOTIFY dpiChanged)
> +Q_PROPERTY(int subPixel READ subPixel WRITE setSubPixel NOTIFY
> subPixelChanged)
> +Q_PROPERTY(int hinting READ hinting WRITE setHinting NOTIFY
> hintingChanged)
Why int here and for the next property and not the enum types directly? This
would avoid the static_cast in KRDB.
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D27156
To: bport, #plasma, ervin, crossi, meven
Cc: usta, 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
bport added inline comments. INLINE COMMENTS > usta wrote in main.qml:242 > I am really away from qml thing but are you sure this one correct ? I mean > isnt this should be > kcm.dpi instead of kcm.fontsAASettings.dpi ?? No dpi is declared inside fontsAASettings REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D27156 To: bport, #plasma, ervin, crossi, meven Cc: usta, 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
bport updated this revision to Diff 75465. bport added a comment. fix coding style REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27156?vs=75446=75465 REVISION DETAIL https://phabricator.kde.org/D27156 AFFECTED FILES kcms/fonts/CMakeLists.txt kcms/fonts/fonts.cpp kcms/fonts/fonts.h kcms/fonts/fontsaasettings.cpp kcms/fonts/fontsaasettings.h kcms/fonts/fontsaasettingsbase.kcfg kcms/fonts/fontsaasettingsbase.kcfgc kcms/fonts/package/contents/ui/main.qml kcms/krdb/krdb.cpp To: bport, #plasma, ervin, crossi, meven 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
bport updated this revision to Diff 75446. bport added a comment. coding style REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27156?vs=75444=75446 REVISION DETAIL https://phabricator.kde.org/D27156 AFFECTED FILES kcms/fonts/CMakeLists.txt kcms/fonts/fonts.cpp kcms/fonts/fonts.h kcms/fonts/fontsaasettings.cpp kcms/fonts/fontsaasettings.h kcms/fonts/fontsaasettingsbase.kcfg kcms/fonts/fontsaasettingsbase.kcfgc kcms/fonts/package/contents/ui/main.qml kcms/krdb/krdb.cpp To: bport, #plasma, ervin, crossi, meven 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
bport updated this revision to Diff 75444. bport added a comment. Fix REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27156?vs=75442=75444 REVISION DETAIL https://phabricator.kde.org/D27156 AFFECTED FILES kcms/fonts/CMakeLists.txt kcms/fonts/fonts.cpp kcms/fonts/fonts.h kcms/fonts/fontsaasettings.cpp kcms/fonts/fontsaasettings.h kcms/fonts/fontsaasettingsbase.kcfg kcms/fonts/fontsaasettingsbase.kcfgc kcms/fonts/package/contents/ui/main.qml kcms/krdb/krdb.cpp To: bport, #plasma, ervin, crossi, meven 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
bport updated this revision to Diff 75442. bport added a comment. Take in consideration Cyril's feedbacks REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27156?vs=75000=75442 REVISION DETAIL https://phabricator.kde.org/D27156 AFFECTED FILES kcms/fonts/CMakeLists.txt kcms/fonts/fonts.cpp kcms/fonts/fonts.h kcms/fonts/fontsaasettings.cpp kcms/fonts/fontsaasettings.h kcms/fonts/fontsaasettingsbase.kcfg kcms/fonts/fontsaasettingsbase.kcfgc kcms/fonts/package/contents/ui/main.qml kcms/krdb/krdb.cpp To: bport, #plasma, ervin, crossi, meven 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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
crossi added inline comments.
INLINE COMMENTS
> fonts.cpp:201
> +Q_ASSERT(dpiItem && dpiWaylandItem && antiAliasingItem);
> +if (dpiItem->isSaveNeeded() || dpiWaylandItem->isSaveNeeded() ||
> antiAliasingItem) {
> +emit aliasingChangeApplied();
antiAliasingItem->isSaveNeeded() ?
> fontssettingsaa.cpp:154
> +
> +KXftConfig::SubPixel::Type spType =
> (KXftConfig::SubPixel::Type)m_subPixel;
> +if (m_subPixelChanged || xft.subPixelTypeHasLocalConfig()) {
`static_cast<>` instead of c-style cast
Also, you can move closer the declaration of `spType`
> fontssettingsaa.cpp:161
> +
> +KXftConfig::Hint::Style hStyle = (KXftConfig::Hint::Style)m_hinting;
> +if (m_hintingChanged || xft.hintStyleHasLocalConfig()) {
`static_cast<>` instead of c-style cast
Also, you can move closer the declaration of `hStyle`
> krdb.cpp:774
>
> -QString hintStyle = generalCfgGroup.readEntry("XftHintStyle",
> "hintslight");
> +KXftConfig::Hint::Style hStyle =
> (KXftConfig::Hint::Style)settingsAA.hinting();
> +QString hintStyle = KXftConfig::toStr(hStyle);
`static_cast<>`
> krdb.cpp:788
>
> -QString subPixel = generalCfgGroup.readEntry("XftSubPixel", "rgb");
> +KXftConfig::SubPixel::Type spType =
> (KXftConfig::SubPixel::Type)settingsAA.subPixel();
> +QString subPixel = KXftConfig::toStr(spType);
`static_cast<>`
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D27156
To: bport, #plasma, ervin, crossi, meven
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
D27156: KCM Fonts port anti aliasing part to KPropertySkeletonItem
bport created this revision. bport added reviewers: Plasma, ervin, crossi, meven. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. bport requested review of this revision. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D27156 AFFECTED FILES kcms/fonts/CMakeLists.txt kcms/fonts/fonts.cpp kcms/fonts/fonts.h kcms/fonts/fontssettingsaa.cpp kcms/fonts/fontssettingsaa.h kcms/fonts/fontssettingsaabase.kcfg kcms/fonts/fontssettingsaabase.kcfgc kcms/fonts/package/contents/ui/main.qml kcms/krdb/krdb.cpp To: bport, #plasma, ervin, crossi, meven Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
