D12849: Improve layout of fonts kcm

rkflx added a comment. In D12849#264040 , @progwolff wrote: > Not sure if this patch should be landed in this half-finished state. Might be an improvement, but might also be perceived as a regression. > I'm away for some hours now, so if

D12849: Improve layout of fonts kcm

progwolff updated this revision to Diff 34350. progwolff added a comment. - i18n REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12849?vs=34349=34350 BRANCH fonts_kcm_layout (branched from master) REVISION DETAIL

D12849: Improve layout of fonts kcm

progwolff updated this revision to Diff 34349. progwolff added a comment. - title case for 'Choose a Font' REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12849?vs=34209=34349 BRANCH fonts_kcm_layout (branched from master) REVISION DETAIL

D12849: Improve layout of fonts kcm

progwolff added a comment. In D12849#263865 , @rkflx wrote: > Did not review the code, but I noticed some warnings in addition to those mentioned above: > > share/kpackage/kcms/kcm_fonts/contents/ui/main.qml:230:25: QML Image: Invalid

D12849: Improve layout of fonts kcm

rkflx added a comment. In D12849#263660 , @progwolff wrote: > That would be this one: > > In D12849#262919 , @progwolff wrote: > > > F5849709: Screenshot_fonts_layout_tabbar.png

D12849: Improve layout of fonts kcm

ngraham added a comment. I know the tabs are currently ugly, but let me explain my reasoning behind suggesting them: It's not a simple "VDG says tabs are bad, don't use them"; scrolling is preferred to a more formal method of organization only when the content itself is naturally

D12849: Improve layout of fonts kcm

progwolff added a comment. That would be this one: In D12849#262919 , @progwolff wrote: > F5849709: Screenshot_fonts_layout_tabbar.png @mart @rkflx @broulik is this okay for you? REPOSITORY

D12849: Improve layout of fonts kcm

abetts added a comment. In D12849#263364 , @progwolff wrote: > In D12849#263302 , @broulik wrote: > > > We were always told by VDG that tabs are bad and a 5 km vertically scrolling list is better

D12849: Improve layout of fonts kcm

progwolff added a comment. In D12849#263302 , @broulik wrote: > We were always told by VDG that tabs are bad and a 5 km vertically scrolling list is better no matter what. So be consistent, please. I thought I read something like this in

D12849: Improve layout of fonts kcm

broulik added a comment. -1 to having tabs We were always told by VDG that tabs are bad and a 5 km vertically scrolling list is bad no matter what. So be consistent, please. REPOSITORY R119 Plasma Desktop BRANCH fonts_kcm_layout (branched from master) REVISION DETAIL

D12849: Improve layout of fonts kcm

ngraham added a comment. Filed some bugs for Kirigami: - https://bugs.kde.org/show_bug.cgi?id=394295 - Tab widget's inactive tabs don't connect with the line below them - https://bugs.kde.org/show_bug.cgi?id=394296 - Kirigami tab widget should decide whether it wants to look like a

D12849: Improve layout of fonts kcm

ngraham added a comment. E, that doesn't look great. Sounds like the view in Kirigami needs some work. @mart Could we have the line extend all the way to the window edges, at least when No Borders is used? REPOSITORY R119 Plasma Desktop BRANCH fonts_kcm_layout (branched from

D12849: Improve layout of fonts kcm

progwolff added a comment. From what I could find there is no TabView in QtQuickControls2 or Kirigami. This is how it's done in Kirigami Gallery: F5849728: kirigami.png I agree that this should be improved, but it's probably better to do this in

D12849: Improve layout of fonts kcm

ngraham added a comment. In D12849#262919 , @progwolff wrote: > F5849709: Screenshot_fonts_layout_tabbar.png Hmm, typically there is a frame around the entire content of the tab, not just at the

D12849: Improve layout of fonts kcm

progwolff added a comment. In D12849#262903 , @ngraham wrote: > @progwolff, what happened to the frame for the tabbed view? Did it the wrong way. Is fixed now. > Also, why is there still a scrollbar there? I have no idea. The

D12849: Improve layout of fonts kcm

progwolff updated this revision to Diff 34209. progwolff added a comment. - fix tabbar frame REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12849?vs=34197=34209 BRANCH fonts_kcm_layout (branched from master) REVISION DETAIL

D12849: Improve layout of fonts kcm

ngraham added a comment. @mart, are you aware of the issues with tabs in Kirigami? @progwolff, what happened to the frame for the tabbed view? Also, why is there still a scrollbar there? REPOSITORY R119 Plasma Desktop BRANCH fonts_kcm_layout (branched from master) REVISION DETAIL

D12849: Improve layout of fonts kcm

progwolff added a comment. Using tabs there are some problems with Kirigami now: file:///usr/lib/qt/qml/org/kde/kirigami.2/ScrollablePage.qml:159: ReferenceError: applicationWindow is not defined ... : QML QQuickLayoutAttached: Binding loop detected for property "alignment"

D12849: Improve layout of fonts kcm

progwolff updated this revision to Diff 34197. progwolff added a comment. - fix button widths REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12849?vs=34196=34197 BRANCH fonts_kcm_layout (branched from master) REVISION DETAIL

D12849: Improve layout of fonts kcm

abetts added a comment. In D12849#262693 , @progwolff wrote: > How about joining the options instead? > > F5849093: Screenshot_fonts_layout_radio.png I like that! REPOSITORY R119 Plasma

D12849: Improve layout of fonts kcm

progwolff updated this revision to Diff 34196. progwolff added a comment. - use tabs REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12849?vs=34191=34196 BRANCH fonts_kcm_layout (branched from master) REVISION DETAIL

D12849: Improve layout of fonts kcm

progwolff added a comment. F5849518: Screenshot_fonts_layout_tab2.png F5849517: Screenshot_fonts_layout_tab1.png REPOSITORY R119 Plasma Desktop BRANCH fonts_kcm_layout (branched from master) REVISION

D12849: Improve layout of fonts kcm

ngraham added a comment. Huge +1 on Henrik's suggestion for removing "Vendor Default" in another patch. This one is looking great though! However, by increasing the vertical size so much, the content is now going to require scrolling at the default System Settings window size of roughly

D12849: Improve layout of fonts kcm

progwolff marked an inline comment as done. progwolff added a comment. F5849439: Screenshot_fonts_layout_radio2.png REPOSITORY R119 Plasma Desktop BRANCH fonts_kcm_layout (branched from master) REVISION DETAIL https://phabricator.kde.org/D12849

D12849: Improve layout of fonts kcm

progwolff updated this revision to Diff 34191. progwolff added a comment. - Use radio buttons. Use spacing instead of Separator. Simplify max(...). REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12849?vs=34065=34191 BRANCH fonts_kcm_layout

D12849: Improve layout of fonts kcm

progwolff added a comment. In D12849#262782 , @rkflx wrote: > In D12849#262781 , @progwolff wrote: > > > In D12849#262777 , @rkflx wrote: > > > > >

D12849: Improve layout of fonts kcm

rkflx added a comment. In D12849#262781 , @progwolff wrote: > In D12849#262777 , @rkflx wrote: > > > BTW, what's the difference between Vendor Default and the regular Defaults button? Either we

D12849: Improve layout of fonts kcm

progwolff added a comment. In D12849#262777 , @rkflx wrote: > > BTW, what's the difference between Vendor Default and the regular Defaults button? Either we provide an option to reset to the default for every single option

D12849: Improve layout of fonts kcm

zzag added inline comments. INLINE COMMENTS > main.qml:34 > id: formLayout > -readonly property int maxImplicitWidth: > Math.max(adjustAllFontsButton.implicitWidth, > Math.max(antiAliasingComboBox.implicitWidth, > Math.max(excludeField.implicitWidth,

D12849: Improve layout of fonts kcm

rkflx added a comment. In D12849#262693 , @progwolff wrote: > How about joining the options instead? > > F5849093: Screenshot_fonts_layout_radio.png Great idea! BTW, what's the difference

D12849: Improve layout of fonts kcm

progwolff added a comment. How about joining the options instead? F5849093: Screenshot_fonts_layout_radio.png REPOSITORY R119 Plasma Desktop BRANCH fonts_kcm_layout (branched from master) REVISION DETAIL https://phabricator.kde.org/D12849

D12849: Improve layout of fonts kcm

abetts added a comment. In D12849#262405 , @progwolff wrote: > You mean like this? > F5848305: Screenshot_fonts_layout_nocheckableitems.png > > Not sure if I like this better... This way "Exclude

D12849: Improve layout of fonts kcm

ngraham added a comment. In D12849#262405 , @progwolff wrote: > Not sure if I like this better... This way "Exclude range" seems to be somewhat unrelated to "Anti-Aliasing". Also, it is not very clear what the checkbox after "Fonts DPI"

D12849: Improve layout of fonts kcm

progwolff added a comment. You mean like this? F5848305: Screenshot_fonts_layout_nocheckableitems.png Not sure if I like this better... This way "Exclude range" seems to be somewhat unrelated to "Anti-Aliasing". Also, it is not very clear what

D12849: Improve layout of fonts kcm

abetts added a comment. In D12849#262088 , @rkflx wrote: > The checkboxes wildly floating around on the left look a bit weird to me. How about this (I guess your original intention was to avoid having checkboxes under the wrong text label?):

D12849: Improve layout of fonts kcm

rkflx added a comment. The checkboxes wildly floating around on the left look a bit weird to me. How about this (I guess your original intention was to avoid having checkboxes under the wrong text label?): Anti-Aliasing: Enabled [ ] Exclude range from 8pt to 15pt

D12849: Improve layout of fonts kcm

mart accepted this revision. mart added a comment. This revision is now accepted and ready to land. for me, good to go REPOSITORY R119 Plasma Desktop BRANCH fonts_kcm_layout (branched from master) REVISION DETAIL https://phabricator.kde.org/D12849 To: progwolff, mart, abetts, ngraham

D12849: Improve layout of fonts kcm

mart added a comment. In D12849#262026 , @progwolff wrote: > In D12849#261837 , @ngraham wrote: > > > We might want to take the opportunity to also center-align the titles, which it better-looking

D12849: Improve layout of fonts kcm

mart added a comment. In D12849#261837 , @ngraham wrote: > Alignment looks great to me! We might want to take the opportunity to also center-align the titles, which it better-looking for these centered formlayout style UIs. And maybe use a bit

D12849: Improve layout of fonts kcm

progwolff added a comment. In D12849#261837 , @ngraham wrote: > We might want to take the opportunity to also center-align the titles, which it better-looking for these centered formlayout style UIs. Section titles in form layouts are

D12849: Improve layout of fonts kcm

ngraham added a comment. Alignment looks great to me! We might want to take the opportunity to also center-align the titles, which it better-looking for these centered formlayout style UIs. And maybe use a bit of extra vertical padding instead of that horizontalline separator? REPOSITORY

D12849: Improve layout of fonts kcm

progwolff created this revision. progwolff added reviewers: mart, abetts, ngraham. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. progwolff requested review of this revision. REVISION SUMMARY Change the layout of the fonts kcm using