ubuntu-touch-coreapps-reviewers team mailing list archive

[Stefano Verzegnassi <stefano92.100@xxxxxxxxx>]

Review: Needs Fixing

Jenkins failed because the 'ownsTexture' property of QSGSimpleTextureNode has been introduced with Qt5.3, but utopic still uses Qt5.3.
I will ask to stop building on utopic since Ubuntu Touch has been built on vivid since some months now.

Anyway, there are few things that IMHO require to be fixed:

- Do we keep 'LOViewer.qml' instead of renaming it to 'Viewer.qml' as discussed?
- Could/Should we restore DEBUG_SHOW_TILE_BORDER ?
- TileItem class is no longer necessary, since we now use SGTileItem. Can we remove it?

For the next question, I've also left some inline comment in the diff. Please check them.
- Instead of commenting debugging lines, could we use an #ifdef DEBUG_VERBOSE in SGTileItem?
- Remove deprecated and commented lines.

Also, in loview.cpp at the beginning of the file, there's a TODO comment about moving to SceneGraph APIs.
You actually did it, and it works greatly. :) We can remove that comment now!

Diff comments:

> 
> === modified file 'src/plugin/libreofficetoolkit-qml-plugin/CMakeLists.txt'
> --- src/plugin/libreofficetoolkit-qml-plugin/CMakeLists.txt	2015-08-25 22:35:28 +0000
> +++ src/plugin/libreofficetoolkit-qml-plugin/CMakeLists.txt	2015-09-04 10:33:13 +0000
> @@ -13,6 +13,7 @@
>      lodocument.cpp
>      loview.cpp
>      tileitem.cpp

TileItem class no longer used.

> +    sgtileitem.cpp
>      ${QML_SRCS}
>  )
>  
> 
> === modified file 'src/plugin/libreofficetoolkit-qml-plugin/loview.cpp'
> --- src/plugin/libreofficetoolkit-qml-plugin/loview.cpp	2015-07-26 17:33:51 +0000
> +++ src/plugin/libreofficetoolkit-qml-plugin/loview.cpp	2015-09-04 10:33:13 +0000
> @@ -29,7 +30,7 @@
>  // updatePaintNode(QSGNode * oldNode, UpdatePaintNodeData * data)
>  
>  LOView::LOView(QQuickItem *parent)
> -    : QQuickPaintedItem(parent)
> +    : QQuickItem(parent) //QQuickPaintedItem(parent)

Remove comment

>      , m_parentFlickable(nullptr)
>      , m_document(nullptr)
>      , m_zoomFactor(1.0)
> @@ -46,17 +47,17 @@
>      connect(&m_updateTimer, SIGNAL(timeout()), this, SLOT(updateVisibleRect()));
>  }
>  
> -void LOView::paint(QPainter *painter)
> -{
> -    Q_FOREACH(TileItem* tile, m_tiles) {
> -        painter->drawImage(tile->area(), tile->texture());
> -        tile->setPainted(true);
> +//void LOView::paint(QPainter *painter)

Remove commented lines.

> +//{
> +//    Q_FOREACH(TileItem* tile, m_tiles) {
> +//        painter->drawImage(tile->area(), tile->texture());
> +//        tile->setPainted(true);
>  
> -#ifdef DEBUG_SHOW_TILE_BORDER
> -        painter->drawRect(tile->area());
> -#endif
> -    }
> -}
> +//#ifdef DEBUG_SHOW_TILE_BORDER
> +//        painter->drawRect(tile->area());
> +//#endif
> +//    }
> +//}
>  
>  // Returns the parent QML Flickable
>  QQuickItem* LOView::parentFlickable() const
> @@ -223,9 +227,9 @@
>          qDebug() << "Creating tile indexed as" << index;
>  #endif
>  
> -        auto tile = new TileItem(rect, m_document);
> -        connect(tile, SIGNAL(textureChanged()), this, SLOT(update()));
> -        tile->requestTexture();
> +        auto tile = new SGTileItem(rect, m_document, this);
> +        // TODO When we should update with SG?

I suspect we don't need to connect it anymore, since LOView class does not run any painting anymore. We can remove safely.

> +        // connect(tile, SIGNAL(textureChanged()), this, SLOT(update()));
>  
>          // Append the tile in the map
>          m_tiles.insert(index, tile);
> 
> === modified file 'src/plugin/libreofficetoolkit-qml-plugin/loview.h'
> --- src/plugin/libreofficetoolkit-qml-plugin/loview.h	2015-07-26 17:33:51 +0000
> +++ src/plugin/libreofficetoolkit-qml-plugin/loview.h	2015-09-04 10:33:13 +0000
> @@ -18,12 +18,14 @@
>  #define LOVIEW_H
>  
>  #include <QQuickPaintedItem>
> +#include <QQuickItem>
>  #include <QTimer>
>  
>  class LODocument;
>  class TileItem;
> +class SGTileItem;
>  
> -class LOView : public QQuickPaintedItem
> +class LOView : public QQuickItem // QQuickPaintedItem

Remove comment.

>  {
>      Q_OBJECT
>      Q_PROPERTY(QQuickItem* parentFlickable READ parentFlickable WRITE setParentFlickable NOTIFY parentFlickableChanged)
> @@ -37,7 +39,7 @@
>      LOView(QQuickItem *parent = 0);
>      ~LOView();
>  
> -    void        paint(QPainter *painter);
> +//    void        paint(QPainter *painter);

Same here.

>  
>      QQuickItem* parentFlickable() const;
>      void        setParentFlickable(QQuickItem* flickable);
> 
> === added file 'src/plugin/libreofficetoolkit-qml-plugin/sgtileitem.cpp'
> --- src/plugin/libreofficetoolkit-qml-plugin/sgtileitem.cpp	1970-01-01 00:00:00 +0000
> +++ src/plugin/libreofficetoolkit-qml-plugin/sgtileitem.cpp	2015-09-04 10:33:13 +0000
> @@ -0,0 +1,73 @@
> +#include "sgtileitem.h"
> +
> +#include "lodocument.h"
> +#include "config.h"
> +
> +SGTileItem::SGTileItem(const QRect &area, LODocument *document, QQuickItem *parent)
> +    : QQuickItem(parent)
> +    , m_area(area)
> +    , m_document(document)
> +    , m_state(SgstInitial)
> +{
> +    setFlag(ItemHasContents, true);
> +}
> +
> +SGTileItem::~SGTileItem()
> +{ }
> +
> +void SGTileItem::dispose()
> +{
> +    if (m_state.loadAcquire() != SgstRendering)
> +        deleteLater();
> +    m_state.storeRelease(SgstDisposed);
> +    // qDebug() << "---- dispose called: " << this << m_state;

Could we use here #ifdef DEBUG_VERBOSE?

> +}
> +
> +QSGNode *SGTileItem::updatePaintNode(QSGNode *oldNode, QQuickItem::UpdatePaintNodeData *)
> +{
> +    QSGSimpleTextureNode* node = static_cast<QSGSimpleTextureNode*>(oldNode);
> +    QQuickWindow* wnd = window();
> +
> +    if (!node && wnd) {
> +        if (this->m_state.loadAcquire() == SgstInitial) {
> +            m_state.storeRelease(SgstRendering);
> +            QtConcurrent::run( [=] {
> +
> +                QImage img;
> +
> +                // By this time already can be disposed, so it's better to ckeck again.
> +                if (this->m_state.loadAcquire() == SgstRendering)
> +                    img = m_document->paintTile(this->area().size(), this->area());
> +
> +                if (this->m_state.loadAcquire() == SgstDisposed)
> +                    qDebug() << "Already disposed:" << m_state.loadAcquire();

Could we use here #ifdef DEBUG_VERBOSE?

> +                QMetaObject::invokeMethod(this, "renderCallback", Q_ARG(QImage, img));
> +
> +            });
> +        } else if (m_state.loadAcquire() == SgstActive) {
> +            QImage image = m_data;
> +            auto texture = wnd->createTextureFromImage(image);
> +            node = new QSGSimpleTextureNode();
> +            node->setTexture(texture);
> +            node->setOwnsTexture(true);
> +            node->setRect(m_area);
> +        }
> +    }
> +
> +    return node;
> +}
> +
> +void SGTileItem::geometryChanged(const QRectF &newGeometry, const QRectF &oldGeometry)
> +{
> +    QQuickItem::geometryChanged(newGeometry, oldGeometry);
> +}
> +
> +void SGTileItem::renderCallback(QImage image)
> +{
> +    if (m_state.loadAcquire() == SgstRendering) {
> +        m_data = image;
> +        m_state.storeRelease(SgstActive);
> +        update();
> +    } else deleteLater();
> +}
> +
> 
> === modified file 'src/plugin/libreofficetoolkit-qml-plugin/tileitem.cpp'
> --- src/plugin/libreofficetoolkit-qml-plugin/tileitem.cpp	2015-07-22 16:44:39 +0000
> +++ src/plugin/libreofficetoolkit-qml-plugin/tileitem.cpp	2015-09-04 10:33:13 +0000
> @@ -160,12 +160,16 @@
>  // Render the texture for this tile.
>  void RenderTask::run()
>  {
> +    // Can cause crash if document is nullptr.

You didn't move this conditional statement in SGTileItem. If the concurrent thread called by ::updatePaintNode() is still running and LODocument does not exists anymore, we could run into the same condition.

> +    if (!m_document)
> +        return;
> +
>  #ifdef DEBUG_TILE_BENCHMARK
>      QElapsedTimer renderTimer;
>      renderTimer.start();
>  #endif
>  
> -    QImage render = this->document()->paintTile(this->area().size(),
> +    QImage render = m_document->paintTile(this->area().size(),
>                                                  this->area());
>  
>      Q_EMIT renderCompleted(render);


-- 
https://code.launchpad.net/~mrqtros/ubuntu-docviewer-app/docviewer-sg-rendering/+merge/270157
Your team Ubuntu Document Viewer Developers is subscribed to branch lp:ubuntu-docviewer-app/reboot.