Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up Comic class and its connections to QThread #202

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Clean up Comic class and its connections to QThread #202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6511e68
Remove redundant Comic::destroyed signal
vedgy Feb 5, 2021
af38fd4
Comic: remove empty destructors and commented out declarations
vedgy Feb 5, 2021
55d2134
Use final/override in Comic and derived classes
vedgy Feb 5, 2021
1a5de3b
ComicController*: connect &Comic::invalidated to &QThread::quit
vedgy Feb 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 4 additions & 16 deletions YACReader/render.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,22 +741,10 @@ void Render::loadComic(const QString &path, int atPage)

void Render::startLoad()
{
QThread *thread = nullptr;

thread = new QThread();

comic->moveToThread(thread);

connect(comic, SIGNAL(errorOpening()), thread, SLOT(quit()), Qt::QueuedConnection);
connect(comic, SIGNAL(errorOpening(QString)), thread, SLOT(quit()), Qt::QueuedConnection);
connect(comic, SIGNAL(imagesLoaded()), thread, SLOT(quit()), Qt::QueuedConnection);
connect(comic, SIGNAL(destroyed()), thread, SLOT(quit()), Qt::QueuedConnection);
connect(comic, SIGNAL(invalidated()), thread, SLOT(quit()), Qt::QueuedConnection);
connect(thread, SIGNAL(started()), comic, SLOT(process()));
connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater()));

if (thread != nullptr)
thread->start();
const auto thread = new QThread();
comic->moveAndConnectToThread(thread);
connect(comic, &QObject::destroyed, thread, &QThread::quit, Qt::QueuedConnection);
thread->start();

invalidate();
loadedComic = true;
Expand Down
18 changes: 3 additions & 15 deletions YACReaderLibrary/server/controllers/v1/comiccontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,22 +55,10 @@ void ComicController::service(HttpRequest &request, HttpResponse &response)
Comic *comicFile = FactoryComic::newComic(libraries.getPath(libraryId) + comic.path);

if (comicFile != nullptr) {
QThread *thread = nullptr;

thread = new QThread();

comicFile->moveToThread(thread);

connect(comicFile, SIGNAL(errorOpening()), thread, SLOT(quit()));
connect(comicFile, SIGNAL(errorOpening(QString)), thread, SLOT(quit()));
connect(comicFile, SIGNAL(imagesLoaded()), thread, SLOT(quit()));
connect(thread, SIGNAL(started()), comicFile, SLOT(process()));
connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater()));

const auto thread = new QThread();
comicFile->moveAndConnectToThread(thread);
comicFile->load(libraries.getPath(libraryId) + comic.path);

if (thread != nullptr)
thread->start();
thread->start();

if (remoteComic) {
QLOG_TRACE() << "remote comic requested";
Expand Down
18 changes: 3 additions & 15 deletions YACReaderLibrary/server/controllers/v2/comiccontroller_v2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,22 +57,10 @@ void ComicControllerV2::service(HttpRequest &request, HttpResponse &response)
Comic *comicFile = FactoryComic::newComic(libraries.getPath(libraryId) + comic.path);

if (comicFile != nullptr) {
QThread *thread = nullptr;

thread = new QThread();

comicFile->moveToThread(thread);

connect(comicFile, SIGNAL(errorOpening()), thread, SLOT(quit()));
connect(comicFile, SIGNAL(errorOpening(QString)), thread, SLOT(quit()));
connect(comicFile, SIGNAL(imagesLoaded()), thread, SLOT(quit()));
connect(thread, SIGNAL(started()), comicFile, SLOT(process()));
connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater()));

const auto thread = new QThread();
comicFile->moveAndConnectToThread(thread);
comicFile->load(libraries.getPath(libraryId) + comic.path);

if (thread != nullptr)
thread->start();
thread->start();

if (remoteComic) {
QLOG_TRACE() << "remote comic requested";
Expand Down
27 changes: 18 additions & 9 deletions common/comic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <QPixmap>
#include <QRegExp>
#include <QString>
#include <QThread>
#include <algorithm>
#include <QDir>
#include <QFileInfoList>
Expand Down Expand Up @@ -120,7 +121,6 @@ Comic::Comic(const QString &pathFile, int atPage)
//-----------------------------------------------------------------------------
Comic::~Comic()
{
emit destroyed();
delete bm;
}
//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -340,6 +340,23 @@ QList<QString> Comic::findValidComicFilesInFolder(const QString &path)
return validComicFiles;
}

void Comic::moveAndConnectToThread(QThread *thread)
{
Q_ASSERT(thread);
moveToThread(thread);

void (Comic::*errorOpening0)() = &Comic::errorOpening;
void (Comic::*errorOpening1)(QString) = &Comic::errorOpening;

connect(this, errorOpening0, thread, &QThread::quit, Qt::QueuedConnection);
connect(this, errorOpening1, thread, &QThread::quit, Qt::QueuedConnection);
connect(this, &Comic::imagesLoaded, thread, &QThread::quit, Qt::QueuedConnection);
connect(this, &Comic::invalidated, thread, &QThread::quit, Qt::QueuedConnection);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was implementing this function, I thought that with the default connection type Qt::AutoConnection, the actual connection type is determined statically inside the QObject::connect() call. But after some debugging, reading Qt source code and rereading the documentation, I realized that this is a truly different connection type:

The connection type is determined when the signal is emitted.

Since both ComicController classes connected to all signals using Qt::AutoConnection, calling this function instead changes their behavior. If this change is undesirable, I can restore the use of Qt::AutoConnection by passing one more argument to this function - qthreadQuitConnectionType.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't want Qt::QueuedConnection in the server side. Do you use YACReader for iOS? Are these changes tested with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't use or test the server. I have noticed the leak fixed in this PR while looking at code patterns similar to the one fixed in #203 and comparing very similar Render and ComicController* connection code. Then decided to unify the code to prevent future accidental divergence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I see that there is no real leak in the current implementation as Server never calls Comic::invalidate(). Though it might call it in the future and connecting to Comic::invalidated still make sense.

So it turns out that this pull request is pure code refactoring, not a fix.


connect(thread, &QThread::started, this, &Comic::process);
connect(thread, &QThread::finished, thread, &QObject::deleteLater);
}

////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -652,10 +669,6 @@ FolderComic::FolderComic(const QString &path, int atPage)
load(path, atPage);
}

FolderComic::~FolderComic()
{
}

bool FolderComic::load(const QString &path, int atPage)
{
_path = path;
Expand Down Expand Up @@ -748,10 +761,6 @@ PDFComic::PDFComic(const QString &path, int atPage)
load(path, atPage);
}

PDFComic::~PDFComic()
{
}

bool PDFComic::load(const QString &path, int atPage)
{
QFileInfo fi(path);
Expand Down
42 changes: 19 additions & 23 deletions common/comic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "pdf_comic.h"
#endif //NO_PDF
class ComicDB;
class QThread;
//#define EXTENSIONS << "*.jpg" << "*.jpeg" << "*.png" << "*.gif" << "*.tiff" << "*.tif" << "*.bmp" Comic::getSupportedImageFormats()
//#define EXTENSIONS_LITERAL << ".jpg" << ".jpeg" << ".png" << ".gif" << ".tiff" << ".tif" << ".bmp" //Comic::getSupportedImageLiteralFormats()
class Comic : public QObject
Expand Down Expand Up @@ -52,7 +53,7 @@ class Comic : public QObject
//Constructors
Comic();
Comic(const QString &pathFile, int atPage = -1);
~Comic();
~Comic() override;
void setup();
//Load pages from file
virtual bool load(const QString &path, int atPage = -1) = 0;
Expand Down Expand Up @@ -83,6 +84,8 @@ class Comic : public QObject
static QList<QString> findValidComicFiles(const QList<QUrl> &list);
static QList<QString> findValidComicFilesInFolder(const QString &path);

void moveAndConnectToThread(QThread *thread);

public slots:
void loadFinished();
void setBookmark();
Expand All @@ -93,9 +96,10 @@ public slots:
void setPageLoaded(int page);
void invalidate();

virtual void process() = 0;

signals:
void invalidated();
void destroyed();
void imagesLoaded();
void imageLoaded(int index);
void imageLoaded(int index, const QByteArray &image);
Expand All @@ -121,39 +125,34 @@ class FileComic : public Comic, public ExtractDelegate
public:
FileComic();
FileComic(const QString &path, int atPage = -1);
~FileComic();
virtual bool load(const QString &path, int atPage = -1);
virtual bool load(const QString &path, const ComicDB &comic);
~FileComic() override;
bool load(const QString &path, int atPage = -1) final;
bool load(const QString &path, const ComicDB &comic) final;
static QList<QString> filter(const QList<QString> &src);

//ExtractDelegate
void fileExtracted(int index, const QByteArray &rawData);
void crcError(int index);
void unknownError(int index);
bool isCancelled();
void fileExtracted(int index, const QByteArray &rawData) override;
void crcError(int index) override;
void unknownError(int index) override;
bool isCancelled() override;

public slots:

void process();
void process() override;
};

class FolderComic : public Comic
{
Q_OBJECT

private:
//void run();

public:
FolderComic();
FolderComic(const QString &path, int atPage = -1);
~FolderComic();

virtual bool load(const QString &path, int atPage = -1);
bool load(const QString &path, int atPage = -1) final;

public slots:

void process();
void process() override;
};

#ifndef NO_PDF
Expand All @@ -171,19 +170,16 @@ class PDFComic : public Comic
Poppler::Document *pdfComic;
#endif
void renderPage(int page);
//void run();

public:
PDFComic();
PDFComic(const QString &path, int atPage = -1);
~PDFComic();

virtual bool load(const QString &path, int atPage = -1);
virtual bool load(const QString &path, const ComicDB &comic);
bool load(const QString &path, int atPage = -1) final;
bool load(const QString &path, const ComicDB &comic) final;

public slots:

void process();
void process() override;
};
#endif //NO_PDF
class FactoryComic
Expand Down