[office/tellico/4.2] src: Ensure image requests are always handled, regardless of success
Robby Stephenson <[email protected]>
| Newsgroups | gmane.comp.kde.cvs |
|---|---|
| Message-ID | <[email protected]> |
Git commit 49b83bdb62fa47151279e8ef80049d1f7ba932ac by Robby Stephenson.
Committed on 16/08/2026 at 18:56.
Pushed by rstephenson into branch '4.2'.
Ensure image requests are always handled, regardless of success
Avoiding stale list of pending image requests in EntryModel when the
request fails to return successfully
M +5 -1 src/images/image.cpp
M +8 -11 src/images/imagefactory.cpp
M +1 -1 src/images/imagefactory.h
M +6 -2 src/models/entrymodel.cpp
M +5 -1 src/models/entrymodel.h
M +9 -8 src/tests/imagejobtest.cpp
M +1 -1 src/tests/imagejobtest.h
M +75 -2 src/tests/tellicomodeltest.cpp
M +1 -0 src/tests/tellicomodeltest.h
M +3 -3 src/tests/tellicoreadtest.cpp
https://invent.kde.org/office/tellico/-/commit/49b83bdb62fa47151279e8ef80049d1f7ba932ac
diff --git a/src/images/image.cpp b/src/images/image.cpp
index 62f142d95..08ab0a4e2 100644
--- a/src/images/image.cpp
+++ b/src/images/image.cpp
@@ -113,7 +113,11 @@ QByteArray Image::outputFormat(const QByteArray& inputFormat) {
if(s_outputFormats.contains(inputFormat.toUpper())) {
return inputFormat;
}
- myWarning() << "writing" << inputFormat << "as PNG";
+ if(inputFormat.isEmpty()) {
+ myDebug() << "defaulting to PNG";
+ } else {
+ myDebug() << "writing" << inputFormat << "as PNG";
+ }
return "PNG";
}
diff --git a/src/images/imagefactory.cpp b/src/images/imagefactory.cpp
index 192c15d41..f25dcb646 100644
--- a/src/images/imagefactory.cpp
+++ b/src/images/imagefactory.cpp
@@ -530,14 +530,12 @@ void ImageFactory::requestImageById(const QString& id_) {
if(hasImageInDirOrMemory(id_)) {
QTimer::singleShot(0, factory, [id_] () {
auto img = factory->addCachedImageImpl(id_, cacheDir());
- if(!img.isNull()) {
- Q_EMIT factory->imageAvailable(id_);
- }
+ Q_EMIT factory->imageRequestFinished(id_, !img.isNull());
});
return;
}
if(factory->d->nullImages.contains(id_)) {
- // don't emit anything
+ Q_EMIT factory->imageRequestFinished(id_, false);
return;
}
const QUrl u(id_);
@@ -549,9 +547,10 @@ void ImageFactory::requestImageById(const QString& id_) {
factory->requestImageByUrlImpl(u, true /* quiet */, QUrl() /* referrer */, linkOnly);
if(!linkOnly) {
myDebug() << "Loading an image url that is not link-only. The image id will get updated.";
- myDebug() << id_;
+ myDebug() << "Existing id:" << id_;
}
}
+ // imageRequestFinished will be emitted by requestImageByUrlImpl ImageJob
return;
}
// real fallback, just as ::imageById() checks, look in fall back directories, just in case
@@ -567,9 +566,7 @@ void ImageFactory::requestImageById(const QString& id_) {
if(realImageDir > -1) {
QTimer::singleShot(0, factory, [id_, realImageDir] () {
auto img = factory->addCachedImageImpl(id_, Tellico::ImageFactory::CacheDir(realImageDir));
- if(!img.isNull()) {
- Q_EMIT factory->imageAvailable(id_);
- }
+ Q_EMIT factory->imageRequestFinished(id_, !img.isNull());
});
return;
}
@@ -817,9 +814,9 @@ void ImageFactory::slotImageJobResult(KJob* job_) {
}
const Data::Image& img = imageJob->image();
if(img.isNull()) {
- myDebug() << "Null image for" << imageJob->url().url(QUrl::PreferLocalFile);
+ myDebug() << "Null image returned for" << imageJob->url().url(QUrl::PreferLocalFile);
d->nullImages.add(imageJob->url().url());
- // don't emit anything
+ Q_EMIT factory->imageRequestFinished(imageJob->url().url(), false);
return;
}
@@ -834,5 +831,5 @@ void ImageFactory::slotImageJobResult(KJob* job_) {
}
s_imageInfoMap.insert(img.id(), Data::ImageInfo(img));
s_imagesToRelease.add(img.id());
- Q_EMIT factory->imageAvailable(img.id());
+ Q_EMIT factory->imageRequestFinished(img.id(), true);
}
diff --git a/src/images/imagefactory.h b/src/images/imagefactory.h
index da7a3d755..1f00d92bb 100644
--- a/src/images/imagefactory.h
+++ b/src/images/imagefactory.h
@@ -180,7 +180,7 @@ public:
static ImageFactory* self();
Q_SIGNALS:
- void imageAvailable(const QString& id);
+ void imageRequestFinished(const QString& id, bool available);
void imageLocationMismatch();
private Q_SLOTS:
diff --git a/src/models/entrymodel.cpp b/src/models/entrymodel.cpp
index 14cc023a0..7d3d9b2be 100644
--- a/src/models/entrymodel.cpp
+++ b/src/models/entrymodel.cpp
@@ -46,7 +46,7 @@ using Tellico::EntryModel;
EntryModel::EntryModel(QObject* parent) : QAbstractItemModel(parent),
m_imagesAreAvailable(true) {
m_checkPix = QIcon::fromTheme(QStringLiteral("checkmark"), QIcon(QLatin1String(":/icons/checkmark")));
- connect(ImageFactory::self(), &ImageFactory::imageAvailable, this, &EntryModel::refreshImage);
+ connect(ImageFactory::self(), &ImageFactory::imageRequestFinished, this, &EntryModel::refreshImage);
}
EntryModel::~EntryModel() = default;
@@ -409,6 +409,7 @@ void EntryModel::setImagesAreAvailable(bool available_) {
if(m_imagesAreAvailable != available_) {
beginResetModel();
m_imagesAreAvailable = available_;
+ m_requestedImages.clear();
endResetModel();
}
}
@@ -441,13 +442,16 @@ QVariant EntryModel::requestImage(Data::EntryPtr entry_, const QString& id_) con
return icon.pixmap(sizes.last());
}
-void EntryModel::refreshImage(const QString& id_) {
+void EntryModel::refreshImage(const QString& id_, bool available_) {
Data::EntryList entries;
for(auto i = m_requestedImages.find(id_); i != m_requestedImages.end() && i.key() == id_; ++i) {
entries += i.value();
}
// remove from request list _before_ emitting dataChanged in case the cache drops the image
m_requestedImages.remove(id_);
+ if(!available_) {
+ myLog() << "Image request failed:" << id_;
+ }
for(const auto& entry : std::as_const(entries)) {
const int idx = m_entries.indexOf(entry);
if(idx >= 0 && !m_fields.isEmpty()) {
diff --git a/src/models/entrymodel.h b/src/models/entrymodel.h
index 8f5fb548f..c96672ccc 100644
--- a/src/models/entrymodel.h
+++ b/src/models/entrymodel.h
@@ -31,6 +31,8 @@
#include <QAbstractItemModel>
#include <QMultiHash>
+class TellicoModelTest; // for testing
+
namespace Tellico {
/**
@@ -39,6 +41,8 @@ namespace Tellico {
class EntryModel : public QAbstractItemModel {
Q_OBJECT
+friend class ::TellicoModelTest;
+
public:
EntryModel(QObject* parent);
virtual ~EntryModel();
@@ -70,7 +74,7 @@ public:
QModelIndex indexFromEntry(Data::EntryPtr entry) const;
private Q_SLOTS:
- void refreshImage(const QString& id);
+ void refreshImage(const QString& id, bool available);
private:
Data::EntryPtr entry(const QModelIndex& index) const;
diff --git a/src/tests/imagejobtest.cpp b/src/tests/imagejobtest.cpp
index 799fef309..663a7b838 100644
--- a/src/tests/imagejobtest.cpp
+++ b/src/tests/imagejobtest.cpp
@@ -62,7 +62,7 @@ void ImageJobTest::initTestCase() {
}
void ImageJobTest::cleanupTestCase() {
- disconnect(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageAvailable,
+ disconnect(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageRequestFinished,
this, &ImageJobTest::slotAvailable);
}
@@ -83,7 +83,8 @@ void ImageJobTest::slotGetResult(KJob* job) {
Q_EMIT exitLoop();
}
-void ImageJobTest::slotAvailable(const QString& id_) {
+void ImageJobTest::slotAvailable(const QString& id_, bool avail_) {
+ Q_UNUSED(avail_)
m_result = 0;
m_imageId = id_;
Q_EMIT exitLoop();
@@ -280,8 +281,8 @@ void ImageJobTest::testNetworkImageInvalid() {
void ImageJobTest::testFactoryRequestLocal() {
QVERIFY(m_imageId.isEmpty());
- QSignalSpy spy(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageAvailable);
- connect(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageAvailable,
+ QSignalSpy spy(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageRequestFinished);
+ connect(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageRequestFinished,
this, &ImageJobTest::slotAvailable);
QUrl u = QUrl::fromLocalFile(QFINDTESTDATA("../../icons/tellico.png"));
@@ -306,8 +307,8 @@ void ImageJobTest::testFactoryRequestLocal() {
void ImageJobTest::testFactoryRequestLocalInvalid() {
QVERIFY(m_imageId.isEmpty());
- QSignalSpy spy(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageAvailable);
- connect(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageAvailable,
+ QSignalSpy spy(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageRequestFinished);
+ connect(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageRequestFinished,
this, &ImageJobTest::slotAvailable);
QCOMPARE(spy.count(), 0);
@@ -335,7 +336,7 @@ void ImageJobTest::testFactoryRequestNetwork() {
if(!hasNetwork()) QSKIP("This test requires network access", SkipSingle);
QVERIFY(m_imageId.isEmpty());
- connect(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageAvailable,
+ connect(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageRequestFinished,
this, &ImageJobTest::slotAvailable);
QUrl u(QStringLiteral("https://tellico-project.org/wp-content/uploads/96-tellico.png"));
@@ -361,7 +362,7 @@ void ImageJobTest::testFactoryRequestNetworkLinkOnly() {
if(!hasNetwork()) QSKIP("This test requires network access", SkipSingle);
QVERIFY(m_imageId.isEmpty());
- connect(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageAvailable,
+ connect(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageRequestFinished,
this, &ImageJobTest::slotAvailable);
QUrl u(QStringLiteral("https://tellico-project.org/wp-content/uploads/96-tellico.png"));
diff --git a/src/tests/imagejobtest.h b/src/tests/imagejobtest.h
index 3a4c4a2fd..721dc6328 100644
--- a/src/tests/imagejobtest.h
+++ b/src/tests/imagejobtest.h
@@ -57,7 +57,7 @@ Q_SIGNALS:
protected Q_SLOTS:
void slotGetResult(KJob *);
- void slotAvailable(const QString&);
+ void slotAvailable(const QString&, bool);
private:
void enterLoop();
diff --git a/src/tests/tellicomodeltest.cpp b/src/tests/tellicomodeltest.cpp
index 32985ac09..1083fa45b 100644
--- a/src/tests/tellicomodeltest.cpp
+++ b/src/tests/tellicomodeltest.cpp
@@ -38,10 +38,12 @@
#include "../models/entryselectionmodel.h"
#include "../collections/bookcollection.h"
#include "../collectionfactory.h"
+#include "../translators/tellicoimporter.h"
#include "../document.h"
#include "../entrygroup.h"
#include "../images/imagefactory.h"
#include "../images/image.h"
+#include "../constants.h"
#include <KLocalizedString>
@@ -123,8 +125,6 @@ void TellicoModelTest::testEntryModel() {
QVERIFY(icon1.isValid());
QVERIFY(!icon1.isNull());
QVERIFY(icon1.canConvert<QIcon>());
- auto& img1 = Tellico::ImageFactory::imageById(imageId);
- QVERIFY(!img1.isNull());
Tellico::FilterRule* rule1 = new Tellico::FilterRule(QStringLiteral("title"),
QStringLiteral("Star Wars"),
@@ -171,6 +171,79 @@ void TellicoModelTest::testEntryModel() {
entryModel.clearSaveState();
}
+void TellicoModelTest::testEntryModelImageRequest() {
+ QUrl imgUrl = QUrl::fromLocalFile(QFINDTESTDATA("../../icons/tellico.png"));
+ imgUrl = imgUrl.adjusted(QUrl::NormalizePathSegments);
+ const QString imageId = imgUrl.url();
+
+ QFile f(QFINDTESTDATA("/data/image_link_test.xml"));
+ QVERIFY(f.exists());
+ QVERIFY(f.open(QIODevice::ReadOnly | QIODevice::Text));
+
+ QTextStream in(&f);
+ QString fileText = in.readAll();
+ fileText.replace(QLatin1String("%COVER%"), imageId);
+
+ Tellico::Import::TellicoImporter importer(fileText);
+ Tellico::Data::CollPtr coll = importer.collection();
+ QVERIFY(coll);
+ QCOMPARE(coll->entries().count(), 1);
+
+ Tellico::Data::EntryPtr entry = coll->entries().at(0);
+ QVERIFY(entry);
+ QCOMPARE(entry->field(QStringLiteral("cover")), imageId);
+
+ Tellico::EntryModel entryModel(this);
+ QVERIFY(entryModel.m_requestedImages.isEmpty());
+ ModelTest test1(&entryModel);
+
+ entryModel.setFields(coll->fields());
+ entryModel.setEntries(coll->entries());
+ QVERIFY(entryModel.m_requestedImages.isEmpty());
+
+ QSignalSpy spy(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageRequestFinished);
+ QModelIndex index = entryModel.index(0, 0);
+
+ auto pixfromVar1 = entryModel.data(index, Tellico::PrimaryImageRole).value<QPixmap>();
+ QVERIFY(!entryModel.m_requestedImages.isEmpty()); // pending image request
+ QVERIFY(!pixfromVar1.isNull()); // default pixmap
+ QVERIFY(spy.wait(2000));
+ QVERIFY(entryModel.m_requestedImages.isEmpty());
+
+ auto pixfromVar2 = entryModel.data(index, Tellico::PrimaryImageRole).value<QPixmap>();
+ QVERIFY(pixfromVar1.cacheKey() != pixfromVar2.cacheKey()); // different image
+
+ auto pix = Tellico::ImageFactory::pixmap(imageId, Tellico::MAX_ENTRY_ICON_SIZE,
+ Tellico::MAX_ENTRY_ICON_SIZE);
+ QVERIFY(pixfromVar1.cacheKey() != pix.cacheKey()); // different image
+ QVERIFY(pixfromVar2.cacheKey() == pix.cacheKey()); // same image
+
+ Tellico::Data::EntryPtr entry2(new Tellico::Data::Entry(coll));
+ entry2->setField(QStringLiteral("title"), QStringLiteral("Star Wars"));
+ coll->addEntries(entry2);
+ entryModel.addEntries({entry2});
+
+ // now remove the image from the cache, but not from the pixmap cache
+ Tellico::ImageFactory::removeImage(imageId, false);
+ entryModel.modifyEntries({entry});
+
+ index = entryModel.index(0, 0);
+ auto pixfromVar3 = entryModel.data(index, Tellico::PrimaryImageRole).value<QPixmap>();
+ QVERIFY(entryModel.m_requestedImages.isEmpty()); // no request since still in cache
+ QVERIFY(pixfromVar2.cacheKey() == pixfromVar3.cacheKey()); // same default image
+
+ entry2->setField(QStringLiteral("cover"), QStringLiteral("https://example.com"));
+ entryModel.modifyEntries({entry2});
+
+ QModelIndex index2 = entryModel.index(1, 0);
+ QVERIFY(index2.isValid());
+ auto noPix = entryModel.data(index2, Tellico::PrimaryImageRole).value<QPixmap>();
+ Q_UNUSED(noPix);
+ QVERIFY(!entryModel.m_requestedImages.isEmpty()); // pending image request
+ QVERIFY(spy.wait(2000));
+ QVERIFY(entryModel.m_requestedImages.isEmpty()); // failed request still removes it from request list
+}
+
void TellicoModelTest::testFilterModel() {
Tellico::FilterModel filterModel(this);
ModelTest test1(&filterModel);
diff --git a/src/tests/tellicomodeltest.h b/src/tests/tellicomodeltest.h
index 12c25a3ae..fade6a464 100644
--- a/src/tests/tellicomodeltest.h
+++ b/src/tests/tellicomodeltest.h
@@ -33,6 +33,7 @@ Q_OBJECT
private Q_SLOTS:
void initTestCase();
void testEntryModel();
+ void testEntryModelImageRequest();
void testFilterModel();
void testGroupModel();
void testSelectionModel();
diff --git a/src/tests/tellicoreadtest.cpp b/src/tests/tellicoreadtest.cpp
index 935f07547..a78fbfa87 100644
--- a/src/tests/tellicoreadtest.cpp
+++ b/src/tests/tellicoreadtest.cpp
@@ -390,7 +390,7 @@ void TellicoReadTest::testLocalImageLink() {
QVERIFY(!Tellico::ImageFactory::self()->hasImageInMemory(imageId));
QVERIFY( Tellico::ImageFactory::self()->hasImageInfo(imageId));
- QSignalSpy spy(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageAvailable);
+ QSignalSpy spy(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageRequestFinished);
Tellico::ImageFactory::self()->requestImageById(imageId);
QVERIFY(spy.wait(2000));
@@ -471,7 +471,7 @@ void TellicoReadTest::testRemoteImageLink() {
QVERIFY(!Tellico::ImageFactory::self()->hasImageInMemory(imageId));
QVERIFY( Tellico::ImageFactory::self()->hasImageInfo(imageId));
- QSignalSpy spy(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageAvailable);
+ QSignalSpy spy(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageRequestFinished);
Tellico::ImageFactory::self()->requestImageById(imageId);
QVERIFY(spy.wait(2000));
@@ -846,7 +846,7 @@ void TellicoReadTest::testImageLocation() {
QVERIFY(!Tellico::ImageFactory::self()->hasImageInMemory(image));
QVERIFY(Tellico::ImageFactory::self()->hasImageInfo(image));
- QSignalSpy spy(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageAvailable);
+ QSignalSpy spy(Tellico::ImageFactory::self(), &Tellico::ImageFactory::imageRequestFinished);
Tellico::ImageFactory::self()->requestImageById(image);
QVERIFY(spy.wait(2000));