Review Request 112375: Use a single frame width for all PM_DefaultFrameWidth
David Edmundson
david at davidedmundson.co.uk
Fri Aug 30 15:11:14 UTC 2013
> On Aug. 30, 2013, 12:43 p.m., Hugo Pereira Da Costa wrote:
> > I respectfully disagree with the change.
> >
> > If I understand the descripion right, the only thing that matters for QtQuick is to change the default returned value (if all else fail) from 1 to FRAME_FRAMEWIDTH. Correct ?
> >
> > The other enumerations, (LineEdit_FrameWidth, ComboBox_FrameWidth, etc.) must stay: even if they have the same values at the moment, this might change in the future (in the near future in fact, since nuno and I want to revisit the metrics, make them more dpi independent, etc.).
> >
> > That the cast wont work for QtQuick is not a good reason to remove it, with all (non quick) applications around, and for which such casts work.
> >
> > Now as for the change needed for QtQuick, this will (must) break (that is: change) all applications that render a frame and don't fall in the cathegories above (think custom widgets). I too have no example of this (but I'm sure there are), and since these are "custom" things, it is normal that they don't show up in oxygen-demo.
> >
> > Please update the change to the "minimal" (namely -> change the default value returned for PM_DefaultFrameWidth), and then, well, we'll need to test ...
> >
>
> David Edmundson wrote:
> I expected this to be a review which started a discussion :)
> It's one that sets precedent for a few other things.
>
> The thing that matters is for QtQuick to get the same width as QLineEdits get. This happens to be FRAME_FRAMEWIDTH now.
>
> The reason I removed the LineEdit_FrameWidth is that if someone later does decide to change the LineEdit_FrameWidth the QtQuickControls will become broken again not matching the desktop counterpart. I'm deliberately taking away that option.
>
> The only way we can support QtQuickControls is to support the lowest common denominator of QStyle that they both follow even if it makes the desktop worse (or at least different).
> This basically means supporting only values in QStyle, and not hacking in our own extra granularity determined by casting widgets.
>
> I appreciate this is a very sucky situation to be in, but if we don't we will we see a very clear discrepancy in the UI depending on which technology happens to be used. I don't think we want that.
>
> By only other options are:
> --
> I could try to add PM_LineEdit and PM_ComboBox into Qt5.2? It would solve this particular issue.
> The base implementation of QStyle can then just call PM_DefaultFrameWidth for backwards compatibility. I'm not sure it would get in as it relies on all styles calling the base implementation for enum values they don't support.
> --
> We make a QML Oxygen theme from 'scratch' a bit like how we have an Oxygen GTK theme.. different tech that happens to look the same.
>
> David Edmundson wrote:
> s/By/My
>
> Christoph Feck wrote:
> Why do QtQuickControls need pixelMetric() for line edits? To find out, how much margin there is around a styled line edit, it should simply use sizeFromContents() and subElementRect().
>
> Hugo Pereira Da Costa wrote:
> "The only way we can support QtQuickControls is to support the lowest common denominator of QStyle that they both follow even if it makes the desktop worse (or at least different).
> This basically means supporting only values in QStyle, and not hacking in our own extra granularity determined by casting widgets."
>
> Two things: this is no hacking. QWidget is an argument passed to the method (though it can be zero), there is no hack in using it, casting it, etc, as far as I can tell. This is a correct use of Qt's API.
>
> Second: "support the lowest common denominator of QStyle that they both follow even"
> I disagree, and besides, I believe this will simply not work. It 'sort of does' in this specific case, but then q_object_cast is used all over the place in oxygen, to, for instance, install event filters, setup/trigger animation, enable mouse-grabbing in empty areas, enable hover on some items for which it was not allowed primarily, etc. Bottomline: I fear that the smallest common denominator that would work identical between QtQuick and QWidget, will be _very_ small, to the point it becomes quite unacceptable; and I 'a priori' do not want to follow this path (especially after all the hours invested in the above).
>
> I aknowledge the will to make QtQuick look identical to QWidgets via oxygen, but imho it must not be done by introducing regressions to the QWidget's rendering. Animations for instance are an interesting case: it is built-in qt quick (if I understand right), while it is enforced via oxygen for widgets. If you drop it from oxygen (for the sake of the common denominator), then you actually introduce inconsistencies between the two.
>
> So that I can see:
>
> 1/ add decent fallbacks (for QtQuick) so that it looks "good enough", and fix them whenever this gets broken (the case you mentionned)
> 2/ implement missing features in QtQuick so that it allows one to look "identical" to what we can do with QWidgets. Naively, I can imagine passing "properties" to the QtQuick widgets, that one can test alongside the failing q_object_cast in oxygen
> 3/ as you say: clone oxygen for QtQuick. But I really would not advocate for this, since it is a pain to maintain, and does not solve anything with respect to solution 1/ concerning fixing things that get broken (inconsitent).
>
> Note that your patch also drops ComboBox_FrameWidth, the latter being also used in a separate enum (PM_ComboBoxFrameWidth).
>
> David Edmundson wrote:
> @Cristoph
> sizeFromContents takes the frame as a parameter which oxygen uses. The frame lineWidth comes from the pixel metric for frames.
> This must also happen for QWidgets as otherwise we wouldn't have a test for QLineEdits inside the pixelMetric function.
>
> From qquickstyleitem.cpp for LineEdits:
> QStyleOptionFrame frame;
> frame.state = m_styleoption->state;
> frame.lineWidth = qApp->style()->pixelMetric(QStyle::PM_DefaultFrameWidth, m_styleoption, 0);
> frame.rect = m_styleoption->rect;
> size = qApp->style()->sizeFromContents(QStyle::CT_LineEdit, &frame, QSize(width, height));
>
>
>
> Hugo Pereira Da Costa wrote:
> @David,
> reading this code, I understand
> - that my "fears" above might be overstated. ;)
> - that this is code you do not have control on, unless committing to Qt directly.
> May I suggest to reformulate the patch to change the existing enums (in oxygenpixelmetrics.h) to
> LineEdit_FrameWidth = Frame_FrameWidth;
> (and same for the others; + add a comment that changing this will breack QtQuickControl)
> so that the "properties" idea is not easily applicable.
> - leave the q_object_cast and enumerations
> - change the default return value to Fram_FrameWidth.
>
> This way, you don't remove functionality, but warn developpers that bad things will happen if they get use (and that discussion should be triggered)
>
>
> Eike Hein wrote:
> It's hard for me to come up with a general opinion on this, I think it probably needs case-by-case examination in a lot of, well, cases. Generally speaking though I don't think we should be dropping features from Oxygen at this time until we've exhausted attempts at upstream solutions. It's unlikely we'll manage to make Controls styling regression-free that way in the 5.2 timeframe of course, but improving Controls for our use cases has to be a long-term goal anyway.
>
> When it comes to lowest common denominator styling, I don't think Oxygen is a good candidate for that. The more similar it looks, the more will feature and visual regressions be felt. If we do want to go with lowest common denominator styling, I think we might need to come up with a new, simplified visual design to go along with it.
>
> As for the specific case, I think the ability to set widget-specific frame widths is actually fairly important to do a good job at form styling. Oxygen doesn't really do much there right now, but I think it's a concern that upstream should answer somehow. So I think the code should probably stay and agree with Hugo on the short-term band-aid of syncing up the default return value ...
Ignore all my original patch.
I can get access to the QML type being used in the theme.
option->styleObject is a QQuickStyleItem. Whilst this is private, it has exported the useful part as a property.
I can get it wtih.
if (option && option->styleObject) {
qDebug() << option->styleObject->property("elementType");
}
This produces output like "spinbox" "edit", "checkbox". etc. I can do this alongside the qobject_cast code to get work out which hints to use.
It slightly complicates the oxygen code, but it means we can fix this without any regressions.
\o/
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112375/#review38928
-----------------------------------------------------------
On Aug. 30, 2013, 11:11 a.m., David Edmundson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112375/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2013, 11:11 a.m.)
>
>
> Review request for Plasma and Hugo Pereira Da Costa.
>
>
> Description
> -------
>
> Use a single frame width for all PM_DefaultFrameWidth
>
> The current code sets a width of 3 for all line edits, combo boxes
> and frames, otherwise it returns a width of 1.
>
> The QtQuickControls engine cannot qobject_cast() the widget so always
> return a frame width of 1 for qtquickcontrol line edits and combo boxes.
>
> This simplifies the code and solves that issue.
>
> I can think of no other way to resolve this without editing Qt, and even then it would be difficult to extend the PixelMetric enum without breaking compatibility.
>
> Note this is potentially a visual change in oxygen, however I have yet to see anything actually different.
>
>
> Diffs
> -----
>
> kstyles/oxygen/oxygenmetrics.h 0643ae5b20d0c9efa328a87e08707cebaabf9f5e
> kstyles/oxygen/oxygenstyle.cpp 86b5cdf3054f5d362d90f0f76c30bfb4f2646911
>
> Diff: http://git.reviewboard.kde.org/r/112375/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> QML After
> http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/spell1.png
> QML_Before
> http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/spell1_1.png
> Normal oxygen demo
> http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/oxygen1.png
>
>
> Thanks,
>
> David Edmundson
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20130830/593986e0/attachment-0001.html>
More information about the Plasma-devel
mailing list