ubuntu-touch-coreapps-reviewers team mailing list archive

[Roman Shchekin <mrqtros@xxxxxxxxx>]

Left few comments.

Diff comments:

> 
> === modified file 'src/plugin/libreofficetoolkit-qml-plugin/loview.cpp'
> --- src/plugin/libreofficetoolkit-qml-plugin/loview.cpp	2015-07-12 21:26:48 +0000
> +++ src/plugin/libreofficetoolkit-qml-plugin/loview.cpp	2015-07-13 12:40:41 +0000
> @@ -195,14 +195,16 @@
>  
>              if (!m_tiles.contains(index)) {
>                  qDebug() << "Creating tile" << x << "x" << y;
> -                TileItem* tile = new TileItem(tileRect, m_document);
> +
> +                auto tile = new TileItem(tileRect, m_document);
> +                tile->requestTexture();

I think that you should put this line ("tile->requestTexture();") after connection of "textureChanged" signal to slot "update". Otherwise rendering can be finished before connection :)

>  
>                  // Append the tile in the map
>                  m_tiles.insert(index, tile);
>  
>                  // Connect the tile to the QQuickPaintedItem's update() slot, so the tile is immediately painted.
>                  qDebug() << "Connecting tile" << x << "x" << y;
> -                connect(tile, SIGNAL(textureUpdated()), this, SLOT(update()));
> +                connect(tile, SIGNAL(textureChanged()), this, SLOT(update()));
>              } else {
>                  // Just some debugging
>                  qDebug() << "tile" << x << "x" << y << "already exists";
> 
> === modified file 'src/plugin/libreofficetoolkit-qml-plugin/tileitem.cpp'
> --- src/plugin/libreofficetoolkit-qml-plugin/tileitem.cpp	2015-07-04 16:00:33 +0000
> +++ src/plugin/libreofficetoolkit-qml-plugin/tileitem.cpp	2015-07-13 12:40:41 +0000
> @@ -26,54 +26,137 @@
>   * TileItem class *
>   ******************/
>  
> -TileItem::TileItem(QRect a, LODocument *doc)
> -    : painted(false)
> -{
> -    area = a;
> -
> -    RenderTask* task = new RenderTask(area, doc);
> +TileItem::TileItem(QRect area, LODocument *document)

Constructor first parameter can be "const QRect& area".

> +    : m_painted(false)
> +    , m_document(nullptr)
> +{
> +    this->setArea(area);
> +    this->setDocument(document);
> +}
> +
> +// Destructor
> +TileItem::~TileItem()
> +{
> +    this->releaseTexture();
> +}
> +
> +QRect TileItem::area() const
> +{
> +    return m_area;
> +}
> +
> +void TileItem::setArea(QRect &area)
> +{
> +    if (m_area == area)
> +        return;
> +
> +    m_area = area;
> +    Q_EMIT areaChanged();
> +}
> +
> +QImage TileItem::texture() const
> +{
> +    return m_texture;
> +}
> +
> +bool TileItem::isPainted() const
> +{
> +    return m_painted;
> +}
> +
> +void TileItem::setPainted(bool isPainted)
> +{
> +    if (m_painted == isPainted)
> +        return;
> +
> +    m_painted = isPainted;
> +    Q_EMIT isPaintedChanged();
> +}
> +
> +LODocument* TileItem::document() const
> +{
> +    return m_document;
> +}
> +
> +void TileItem::setDocument(LODocument* document)
> +{
> +    if (m_document == document)
> +        return;
> +
> +    m_document = document;
> +    Q_EMIT documentChanged();
> +}
> +
> +void TileItem::requestTexture()
> +{
> +    auto task = new RenderTask(this->area(), this->document());
>      connect(task, SIGNAL(renderCompleted(QImage)), this, SLOT(updateTexture(QImage)));
>  
>      task->setAutoDelete(true);
>      QThreadPool::globalInstance()->start(task);
>  }
>  
> -// Destructor
> -TileItem::~TileItem()
> -{
> -    this->releaseTexture();
> -}
> -
>  // Free memory used by the texture
>  void TileItem::releaseTexture()
>  {
> -    if (texture.isNull())
> +    if (m_texture.isNull())
>          return;
>  
> -    texture = QImage();
> +    m_texture = QImage();
> +    Q_EMIT textureChanged();
>  }
>  
>  // This is a slot, connect to renderCompleted() signal from RenderTask class.
>  void TileItem::updateTexture(QImage t)
>  {
> -    qDebug() << "Updating texture";
> -    texture = t;
> -    Q_EMIT textureUpdated();
> +    m_texture = t;
> +    Q_EMIT textureChanged();
>  }
>  
>  /* ******************
>   * RenderTask class *
>   ********************/
>  
> -RenderTask::RenderTask(QRect area, LODocument *doc)
> -{
> -    m_document = doc;
> +RenderTask::RenderTask(QRect area, LODocument *document)

const QRect& area

> +{
> +    this->setArea(area);
> +    this->setDocument(document);
> +}
> +
> +
> +QRect RenderTask::area() const
> +{
> +    return m_area;
> +}
> +
> +void RenderTask::setArea(QRect &area)
> +{
> +    if (m_area == area)
> +        return;
> +
>      m_area = area;
> +    Q_EMIT areaChanged();
> +}
> +
> +LODocument* RenderTask::document() const
> +{
> +    return m_document;
> +}
> +
> +void RenderTask::setDocument(LODocument* document)
> +{
> +    if (m_document == document)
> +        return;
> +
> +    m_document = document;
> +    Q_EMIT documentChanged();
>  }
>  
>  // Render the texture for this tile.
>  void RenderTask::run()
>  {
> -    QImage render = m_document->paintTile(m_area.size(), m_area);
> +    QImage render = this->document()->paintTile(this->area().size(),
> +                                                this->area());
> +
>      Q_EMIT renderCompleted(render);
>  }


-- 
https://code.launchpad.net/~verzegnassi-stefano/ubuntu-docviewer-app/lo-tileitem-class-code/+merge/264550
Your team Ubuntu Document Viewer Developers is requested to review the proposed merge of lp:~verzegnassi-stefano/ubuntu-docviewer-app/lo-tileitem-class-code into lp:~ubuntu-docviewer-dev/ubuntu-docviewer-app/lo-tiled-rendering.