Resizable spinnies #1079

Be-ing · 2016-12-16T16:41:09Z

Working on PR #940, I realized spinnies were set to a fixed size in C++ and wouldn't scale. This PR rerenders SVGs to QPixmaps when spinnies are resized. I avoided rendering the SVGs in WSpinny::paintEvent() for efficiency.

This is not ready for merge. There are two big issues I could use help with, and their solutions might be related:

The QPixmaps are not rendered when Mixxx is loaded. The debugging statement in WSpinny::resizeEvent() shows that calling resize() in LegacySkinParser::parseSpinny() executes WSpinny::resizeEvent(), and even though the height and width are not equal, resizeEvent() isn't being called again.
The code at the top of resizeEvent() forces the spinny to stay a circle, but the parent WidgetGroup is not laid out with the new square size. I have tried calling updateGeometry() in resizeEvent(), but this creates an infinite loop. Searching how to do this online suggests implementing a custom QLayout for the widget, but using QLayout for a single widget doesn't seem appropriate. Suggestions?

rryan

Yea, this is definitely tricky due to unfortunate problems with Qt's layout system. I'll have to play around with it -- it's been a while since I last tried to keep a widget square. I think it may require something ugly like wrapping WSpinny in a widget that has a custom QLayout that forces the spinny to stay square.

rryan · 2016-12-16T17:22:06Z

src/skin/legacyskinparser.cpp

@@ -1112,6 +1112,8 @@ QWidget* LegacySkinParser::parseSpinny(const QDomElement& node) {
        return dummy;
    }
    commonWidgetSetup(node, spinny);
+    // Pixmaps are rendered from SVGs upon resizing, so initialize the pixmaps
+    spinny->resize(spinny->size());


This can stall the skin parsing process since it triggers a bunch of QtGui code (like layout) before the skin is done being created. Instead, we could do this inside WSpinny with a helper method that generates the pixmaps, then call that in setup() and resizeEvent().

I tried calling it in WSpinny::setup(), but that didn't work either. IIRC WSpinny's initial size was not set up at that point, but I'll try again.

rryan · 2016-12-16T17:22:11Z

src/skin/pixmapsource.cpp

@@ -48,6 +50,31 @@ void PixmapSource::setSVG(const QByteArray& content) {
    m_eType = SVG;
 }

+QPixmap PixmapSource::toPixmap(const QSize& size) {


This probably belongs on Paintable in src/widget/wpixmapstore.h/cpp. PixmapSource is a description of a pixmap/svg, not a means to render it. All the rendering code is currently kept in Paintable.

I first looked into using Paintable's draw methods inside WSpinny::paintEvent(), but yeah moving the toPixmap() function to Paintable and calling that from WSpinny::resizeEvent() seems like a better idea than the current state.

Ah, I think I understand how to use Paintable::draw() for this. Create a QPainter in WSpinny::resizeEvent() that paints onto the QPixmap members of WSpinny instead of onto the widget like other parts of the skin code.

rryan · 2016-12-16T17:29:09Z

src/widget/wspinny.cpp

+             << "old size:" << re->oldSize();
+    // Keep it square
+    if (height() < width()) {
+        resize(height(), height());


I think even with this check it isn't safe. The parent widget's layout has the final say over our size, so it can modify the request in a way that would trigger an infinite loop.

rryan · 2016-12-16T17:38:54Z

src/widget/wspinny.h

@@ -60,6 +60,7 @@ class WSpinny : public QGLWidget, public WBaseWidget, public VinylSignalQualityL
    void showEvent(QShowEvent* event) override;
    void hideEvent(QHideEvent* event) override;
    bool event(QEvent* pEvent) override;
+    QSize sizeHint() const override;


May also want to define heightForWidth.
https://doc.qt.io/qt-4.8/qwidget.html#heightForWidth

I tried. It didn't do anything.

rryan · 2016-12-16T17:41:39Z

src/widget/wspinny.cpp

-        m_ghostImageScaled = m_pGhostImage->scaled(
-                size(), Qt::KeepAspectRatio, Qt::SmoothTransformation);
-    }
+    m_bgSource = context.getPixmapSource(


We should keep the legacy behavior (bg scalemode is FIXED, then setFixedSize, otherwise set MinimumExpanding) otherwise this can break old skins.

How could we determine when to revert to legacy behavior?

rryan · 2016-12-16T17:52:27Z

src/widget/wspinny.cpp

+    // specify deck color in QSS
+    m_loadedCoverScaled = scaledCoverArt(m_loadedCover);
+
+    m_bgPixmap = m_bgSource.toPixmap(size());


This is going to reload the pixmap from file / parse the SVG every time (which will block the GUI thread every time the spinny gets resized). Instead, let's keep a Paintable in the WSpinny for each of the originals and use Paintable::draw to render the pixmap at the desired size (that way it's all CPU + some mallocs).

rryan · 2016-12-16T18:02:09Z

src/widget/wspinny.h

-    QImage* m_pGhostImage;
-    QImage m_ghostImageScaled;
+
+    QPixmap m_bgPixmap;


There is some history for why WSpinny uses QImage.

https://bugs.launchpad.net/mixxx/+bug/981210
a15c268
#440

@daschuer -- do you still have this netbook? I wonder if this bug in Qt 4.6.2 was fixed in a later version.

Be-ing · 2016-12-16T19:07:39Z

I think it may require something ugly like wrapping WSpinny in a widget that has a custom QLayout that forces the spinny to stay square.

I'm afraid you may be right. :/

daschuer · 2016-12-16T19:28:32Z

@daschuer -- do you still have this netbook? I wonder if this bug in Qt 4.6.2 was fixed in a later version.

Yes it is still alive, running Trusty and QT 4.8.5. Mixxx is still usable. :-)

This keeps the SVG/PNG in memory rather than reading from disk each rerender.

rryan · 2016-12-20T16:08:28Z

@daschuer -- do you still have this netbook? I wonder if this bug in Qt 4.6.2 was fixed in a later version.
Yes it is still alive, running Trusty and QT 4.8.5. Mixxx is still usable. :-)

Hm, so as long as we're supporting Trusty, we can't make the switch here to QPixmap here without reviving the bug. When should we end Trusty support?

ywwg · 2016-12-20T16:15:48Z

trusty came out in 2014 and is an LTS so it's technically supported until 2019

rryan · 2016-12-20T16:17:30Z

trusty came out in 2014 and is an LTS so it's technically supported until 2019

Indeed -- that doesn't mean we have to support it through 2019 though. Mixxx 1.10 will work fine on Trusty through 2019 :).

http://packages.ubuntu.com/trusty/sound/mixxx

Be-ing · 2016-12-20T16:31:43Z

From what I can tell, what Daniel said in this bug comment still stands. Looks like it will be easy to change to using QImage.

ywwg · 2016-12-20T16:52:24Z

Mixxx 1.10 will work fine on Trusty through 2019 :).

that seems sufficient

avoid reintroducing https://bugs.launchpad.net/mixxx/+bug/981210

Be-ing · 2016-12-20T17:58:35Z

I think the resizing looks slightly smoother for my system using QImage instead of QPixmap. I'm using a Core i5 2410M with Intel HD Graphics 3000 using the default Intel Linux graphic driver.

Both issues mentioned at the top of this PR still remain.

sblaisot · 2016-12-20T19:47:11Z

trusty came out in 2014 and is an LTS so it's technically supported until 2019

Indeed -- that doesn't mean we have to support it through 2019 though. Mixxx 1.10 will work fine on Trusty through 2019 :).

Are you suggesting to remove Windows XP support ? \o/

rryan · 2016-12-21T17:52:21Z

Are you suggesting to remove Windows XP support ? \o/

Yes, for Mixxx 2.2! Qt 5 dropped support for XP, so once we switch we have to.

Be-ing · 2016-12-28T16:47:11Z

it may require something ugly like wrapping WSpinny in a widget that has a custom QLayout that forces the spinny to stay square.

I tried that and the result is the same. I can get the QLayout's setGeometry() method to resize the inner widget, but I don't know how to get the outer widget to shrink to the size of the inner widget. Maybe a subclass of WidgetGroup with a custom layout is needed for the container holding the waveform and the spinny?

Be-ing · 2016-12-28T20:27:58Z

src/widget/wspinny.cpp

+
+    if (rect.height() > rect.width()) {
+        m_pSpinnyItem->setGeometry(QRect(0, 0, rect.width(), rect.width()));
+        QLayout::setGeometry(QRect(0, 0, rect.width(), rect.width()));


I haven't found any documentation on what calling QLayout::setGeometry() in a QLayout subclass' setGeometry method does, but I see it in all of Qt's examples.

Shade does not specify a mask image

rerender SVGs upon resizing spinnies

70e48d5

rryan reviewed Dec 16, 2016

View reviewed changes

replace PixmapSource::toPixmap() with Paintable::renderToPixmap()

2c5a26f

This keeps the SVG/PNG in memory rather than reading from disk each rerender.

revert to using QImage instead of QPixmap

170aa54

avoid reintroducing https://bugs.launchpad.net/mixxx/+bug/981210

Be-ing commented Dec 28, 2016

View reviewed changes

Be-ing mentioned this pull request Feb 5, 2017

Deere 2.1 #940

Merged

12 tasks

Merge remote-tracking branch 'upstream/master' into resizable_spinnies

38d605c

Be-ing force-pushed the resizable_spinnies branch from 2ac3a6f to 38d605c Compare March 5, 2017 17:58

fix segfault when no mask image is specified for a spinny

b8db7c0

Shade does not specify a mask image

Be-ing closed this Apr 28, 2017

Be-ing mentioned this pull request Oct 19, 2017

Deere 2.1 part 2 #1343

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resizable spinnies #1079

Resizable spinnies #1079

Be-ing commented Dec 16, 2016 •

edited

Loading

rryan left a comment

rryan Dec 16, 2016

Be-ing Dec 16, 2016

rryan Dec 16, 2016

Be-ing Dec 16, 2016

Be-ing Dec 16, 2016

rryan Dec 16, 2016

rryan Dec 16, 2016

Be-ing Dec 16, 2016

rryan Dec 16, 2016

Be-ing Dec 20, 2016

rryan Dec 16, 2016

rryan Dec 16, 2016

Be-ing commented Dec 16, 2016

daschuer commented Dec 16, 2016 •

edited

Loading

rryan commented Dec 20, 2016

ywwg commented Dec 20, 2016

rryan commented Dec 20, 2016

Be-ing commented Dec 20, 2016

ywwg commented Dec 20, 2016

Be-ing commented Dec 20, 2016 •

edited

Loading

sblaisot commented Dec 20, 2016 •

edited

Loading

rryan commented Dec 21, 2016

Be-ing commented Dec 28, 2016 •

edited

Loading

Be-ing Dec 28, 2016

Resizable spinnies #1079

Resizable spinnies #1079

Conversation

Be-ing commented Dec 16, 2016 • edited Loading

rryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Dec 16, 2016

daschuer commented Dec 16, 2016 • edited Loading

rryan commented Dec 20, 2016

ywwg commented Dec 20, 2016

rryan commented Dec 20, 2016

Be-ing commented Dec 20, 2016

ywwg commented Dec 20, 2016

Be-ing commented Dec 20, 2016 • edited Loading

sblaisot commented Dec 20, 2016 • edited Loading

rryan commented Dec 21, 2016

Be-ing commented Dec 28, 2016 • edited Loading

Choose a reason for hiding this comment

Be-ing commented Dec 16, 2016 •

edited

Loading

daschuer commented Dec 16, 2016 •

edited

Loading

Be-ing commented Dec 20, 2016 •

edited

Loading

sblaisot commented Dec 20, 2016 •

edited

Loading

Be-ing commented Dec 28, 2016 •

edited

Loading