> On May 9, 2015, 1:18 p.m., Eike Hein wrote: > > Regressions just going by the screenshot: > > * Doesn't use form layouting (e.g. the font box labels aren't > > right-aligned). > > * The font box height doesn't match the button heights anymore. > > * Missing colons in the text labels. > > * Wonky margin between combo box label and combo box. > > * No keyboard accelerators. > > > > General conceptual problems with this port: > > * Poor QStyle support, e.g. drag on empty space in Oxygen/Breeze doesn't > > work with this toolkit. > > * IIRC QStyle background support (like Oxygen's radial gradient) doesn't > > work either, don't remember though. > > > > Additional problems after trying it out: > > * Poor load performance - loading the KCM from the System Settings main > > page is significantly slower than the old KCM and involves some flicker. > > * The anti-aliasing config dialog has a white/unstyled background and > > improper dialog margins. There's also a ton of crap margins/positioning > > around widget in its form. > > * The "Hinting style" combo box popup has wonky margins between the radio > > buttons and the text labels, and a grey background when it's supposed to be > > white. > > > > What's the motivation behind this port? What user problem does it solve? > > How does it make this product better? Remember to not just throw code over > > the fence; you're supposed to argue for why a change is a good idea on > > review board. Your review request doesn't adequately answer any of these > > questions. Note that "but other KCMs use Qt Quick and have similar > > problems" is no argument for making this one worse (but it's an argument > > for fixing the others by whatever means). > > Eike Hein wrote: > Sorry, the above was destroyed by markdown parsing: > > Regressions just going by the screenshot: > - Doesn't use form layouting (e.g. the font box labels aren't > right-aligned). > - The font box height doesn't match the button heights anymore. > - Missing colons in the text labels. > - Wonky margin between combo box label and combo box. > - No keyboard accelerators. > > General conceptual problems with this port: > - Poor QStyle support, e.g. drag on empty space in Oxygen/Breeze doesn't > work with this toolkit. > - IIRC QStyle background support (like Oxygen's radial gradient) doesn't > work either, don't remember though. > > Additional problems after trying it out: > - Poor load performance - loading the KCM from the System Settings main > page is significantly slower than the old KCM and involves some flicker. > - The anti-aliasing config dialog has a white/unstyled background and > improper dialog margins. There's also a ton of crap margins/positioning > around widget in its form. > - The "Hinting style" combo box popup has wonky margins between the radio > buttons and the text labels, and a grey background when it's supposed to be > white. > > What's the motivation behind this port? What user problem does it solve? > How does it make this product better? Remember to not just throw code over > the fence; you're supposed to argue for why a change is a good idea on review > board. Your review request doesn't adequately answer any of these questions. > Note that "but other KCMs use Qt Quick and have similar problems" is no > argument for making this one worse (but it's an argument for fixing the > others by whatever means). > > Marco Martin wrote: > so, we talked a bit about it in hangout, the solution will be: > * do a new branch of master (called like QmlKCMs) > * merge this and the cursortheme one in such branch > * do any eventual new one in the branch > * master stays as it is for the next future > * (even ask distributions to package such branch as an experimental, non > default alternative to make testing easy) > > any eventual qml/qtquickcontrols but that comes out from doing that is > attempted to be fixed upstream > the one big branch will be merged when good enough.
^ Sounds good - Eike ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123682/#review80124 ----------------------------------------------------------- On May 8, 2015, 12:39 p.m., Antonis Tsiapaliokas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/123682/ > ----------------------------------------------------------- > > (Updated May 8, 2015, 12:39 p.m.) > > > Review request for Plasma. > > > Repository: plasma-desktop > > > Description > ------- > > This patch ports the kcm fonts to QML. > > > Diffs > ----- > > kcms/fonts/CMakeLists.txt d73636e > kcms/fonts/fonts.cpp 74da799 > kcms/fonts/fonts.desktop 1cdad40 > kcms/fonts/fonts.h d98bbe2 > kcms/fonts/kcm_fonts.desktop PRE-CREATION > kcms/fonts/package/contents/ui/main.qml PRE-CREATION > kcms/fonts/package/metadata.desktop PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/123682/diff/ > > > Testing > ------- > > Everything works execpt from the ComboBox and the FontDialog ("Configure > Font"). > > * FontDialog > > If you open the kcm inside from the system settings, everything is ok. > > If you use kcmshell5 fonts, the FontDialog is opening behing the kcm window. > In order to solve this issue we must use the setTransientParent, but how can > i do that in the FontDialog? > > * ComboBox > > If you open the kcm with the "kcmshell5 fonts", the dropdown menu renders > fine. > > But if you open it inside from the system settings, the dropdown menu, renders > in the left of the ComboBox. > > > Also these two signals (main.qml line 295) > > onDpiChanged > onAliasingChanged > > are being emitted but the kcm.needsSave doesn't work... > > > File Attachments > ---------------- > > fonts qml > > https://git.reviewboard.kde.org/media/uploaded/files/2015/05/08/833710e7-9569-4cd3-a02f-3ebe95a1c914__kcm_fonts_qml.png > > > Thanks, > > Antonis Tsiapaliokas > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel