Lazily load images on paint, not layout #3950
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm going to run this PR as my daily driver for a while, and I'd urge others to do the same To test a PR build, you can browse to the linked PR, click the "Checks" tab, then select the "Build" job on the left side, then scroll down to find the build that fits your platform. Note that you need to be signed into GitHub to download from that page. |
|
FFZ badges are missing their background on Windows Interestingly does NOT apply to custom VIP badges, they continue to work fine. |
|
I've run this for a week now, only thing I've noticed is what Felanbird mentioned, some special badges don't render their background properly |
Nerixyz
reviewed
Oct 6, 2023
Wissididom
reviewed
Oct 7, 2023
pajlada
requested changes
Oct 7, 2023
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/messages/MessageElement.cpp
Outdated
| auto image = | ||
| emote->images.getImageOrLoaded(container.getScale()); | ||
| if (!image->isEmpty()) | ||
| float overallScale = |
There was a problem hiding this comment.
warning: variable 'overallScale' of type 'float' can be declared 'const' [misc-const-correctness]
Suggested change
| float overallScale = | |
| float const overallScale = |
src/messages/MessageElement.cpp
Outdated
| } | ||
|
|
||
| int currentWidth = metrics.horizontalAdvance(currentText); |
There was a problem hiding this comment.
warning: variable 'currentWidth' of type 'int' can be declared 'const' [misc-const-correctness]
Suggested change
| int currentWidth = metrics.horizontalAdvance(currentText); | |
| int const currentWidth = metrics.horizontalAdvance(currentText); |
| } | ||
|
|
||
| void PriorityImageLayoutElement::addCopyTextToString(QString &str, | ||
| uint32_t from, |
There was a problem hiding this comment.
warning: parameter 'from' is unused [misc-unused-parameters]
Suggested change
| uint32_t from, | |
| uint32_t /*from*/, |
|
|
||
| void PriorityImageLayoutElement::addCopyTextToString(QString &str, | ||
| uint32_t from, | ||
| uint32_t to) const |
There was a problem hiding this comment.
warning: parameter 'to' is unused [misc-unused-parameters]
Suggested change
| uint32_t to) const | |
| uint32_t /*to*/) const |
| } | ||
|
|
||
| void PriorityImageLayoutElement::paint(QPainter &painter, | ||
| const MessageColors &messageColors) |
There was a problem hiding this comment.
warning: parameter 'messageColors' is unused [misc-unused-parameters]
Suggested change
| const MessageColors &messageColors) | |
| const MessageColors & /*messageColors*/) |
| return this->getRect().left(); | ||
| } | ||
| else if (index == 1) | ||
| { |
There was a problem hiding this comment.
warning: repeated branch in conditional chain [bugprone-branch-clone]
{
^Additional context
src/messages/layouts/MessageLayoutElement.cpp:410: end of the original
}
^src/messages/layouts/MessageLayoutElement.cpp:412: clone 1 starts here
{
^
| auto image = | ||
| emote->images.getImageOrLoaded(container.getScale()); | ||
| if (!image->isEmpty()) | ||
| float overallScale = getSettings()->emoteScale.getValue() * |
There was a problem hiding this comment.
warning: variable 'overallScale' of type 'float' can be declared 'const' [misc-const-correctness]
Suggested change
| float overallScale = getSettings()->emoteScale.getValue() * | |
| float const overallScale = getSettings()->emoteScale.getValue() * |
| } | ||
|
|
||
| int currentWidth = metrics.horizontalAdvance(currentText); |
There was a problem hiding this comment.
warning: variable 'currentWidth' of type 'int' can be declared 'const' [misc-const-correctness]
Suggested change
| int currentWidth = metrics.horizontalAdvance(currentText); | |
| int const currentWidth = metrics.horizontalAdvance(currentText); |
|
emote scale passed to diff --git a/src/messages/MessageElement.cpp b/src/messages/MessageElement.cpp
index 49d49aacb..ec9bd5c5a 100644
--- a/src/messages/MessageElement.cpp
+++ b/src/messages/MessageElement.cpp
@@ -27,7 +27,7 @@ namespace {
return;
}
- auto size = priority->firstLoadedImageSize() * container.getScale();
+ auto size = priority->firstLoadedImageSize() * scale;
container.addElement((new PriorityImageLayoutElement(
element, std::move(*priority), size))background is missing for FFZ badges. because diff --git a/src/messages/MessageElement.cpp b/src/messages/MessageElement.cpp
index 49d49aacb..f3cbb6950 100644
--- a/src/messages/MessageElement.cpp
+++ b/src/messages/MessageElement.cpp
@@ -19,7 +19,8 @@ namespace chatterino {
namespace {
void addImageSetToContainer(MessageLayoutContainer &container,
MessageElement &element,
- const ImageSet &imageSet, float scale)
+ const ImageSet &imageSet, float scale,
+ std::optional<QColor> bgColor = std::nullopt)
{
auto priority = imageSet.getPriority(scale);
if (!priority)
@@ -30,7 +31,7 @@ namespace {
auto size = priority->firstLoadedImageSize() * container.getScale();
container.addElement((new PriorityImageLayoutElement(
- element, std::move(*priority), size))
+ element, std::move(*priority), size, bgColor))
->setLink(element.getLink()));
}
@@ -241,12 +242,6 @@ void EmoteElement::addToContainer(MessageLayoutContainer &container,
}
}
-MessageLayoutElement *EmoteElement::makeImageLayoutElement(
- const ImagePtr &image, const QSize &size)
-{
- return new ImageLayoutElement(*this, image, size);
-}
-
LayeredEmoteElement::LayeredEmoteElement(
std::vector<LayeredEmoteElement::Emote> &&emotes, MessageElementFlags flags,
const MessageColor &textElementColor)
@@ -287,10 +282,9 @@ void LayeredEmoteElement::addToContainer(MessageLayoutContainer &container,
individualSizes.push_back(QSize(img->width(), img->height()) *
overallScale);
}
-
- container.addElement(this->makeImageLayoutElement(
- images, individualSizes, largestSize)
- ->setLink(this->getLink()));
+ auto *newElement = new LayeredImageLayoutElement(
+ *this, images, individualSizes, largestSize);
+ container.addElement(newElement->setLink(this->getLink()));
}
else
{
@@ -320,13 +314,6 @@ std::vector<ImagePtr> LayeredEmoteElement::getLoadedImages(float scale)
return res;
}
-MessageLayoutElement *LayeredEmoteElement::makeImageLayoutElement(
- const std::vector<ImagePtr> &images, const std::vector<QSize> &sizes,
- QSize largestSize)
-{
- return new LayeredImageLayoutElement(*this, images, sizes, largestSize);
-}
-
void LayeredEmoteElement::updateTooltips()
{
if (!this->emotes_.empty())
@@ -412,9 +399,11 @@ std::vector<LayeredEmoteElement::Emote> LayeredEmoteElement::getUniqueEmotes()
}
// BADGE
-BadgeElement::BadgeElement(const EmotePtr &emote, MessageElementFlags flags)
+BadgeElement::BadgeElement(const EmotePtr &emote, MessageElementFlags flags,
+ std::optional<QColor> bgColor)
: MessageElement(flags)
, emote_(emote)
+ , bgColor_(bgColor)
{
this->setTooltip(emote->tooltip.string);
}
@@ -425,7 +414,7 @@ void BadgeElement::addToContainer(MessageLayoutContainer &container,
if (flags.hasAny(this->getFlags()))
{
addImageSetToContainer(container, *this, this->emote_->images,
- container.getScale());
+ container.getScale(), this->bgColor_);
}
}
@@ -434,34 +423,13 @@ EmotePtr BadgeElement::getEmote() const
return this->emote_;
}
-MessageLayoutElement *BadgeElement::makeImageLayoutElement(
- const ImagePtr &image, const QSize &size)
-{
- auto element =
- (new ImageLayoutElement(*this, image, size))->setLink(this->getLink());
-
- return element;
-}
-
// MOD BADGE
ModBadgeElement::ModBadgeElement(const EmotePtr &data,
MessageElementFlags flags_)
- : BadgeElement(data, flags_)
+ : BadgeElement(data, flags_, {"#34AE0A"})
{
}
-MessageLayoutElement *ModBadgeElement::makeImageLayoutElement(
- const ImagePtr &image, const QSize &size)
-{
- static const QColor modBadgeBackgroundColor("#34AE0A");
-
- auto element = (new ImageWithBackgroundLayoutElement(
- *this, image, size, modBadgeBackgroundColor))
- ->setLink(this->getLink());
-
- return element;
-}
-
// VIP BADGE
VipBadgeElement::VipBadgeElement(const EmotePtr &data,
MessageElementFlags flags_)
@@ -469,31 +437,11 @@ VipBadgeElement::VipBadgeElement(const EmotePtr &data,
{
}
-MessageLayoutElement *VipBadgeElement::makeImageLayoutElement(
- const ImagePtr &image, const QSize &size)
-{
- auto element =
- (new ImageLayoutElement(*this, image, size))->setLink(this->getLink());
-
- return element;
-}
-
// FFZ Badge
FfzBadgeElement::FfzBadgeElement(const EmotePtr &data,
MessageElementFlags flags_, QColor color_)
- : BadgeElement(data, flags_)
- , color(std::move(color_))
-{
-}
-
-MessageLayoutElement *FfzBadgeElement::makeImageLayoutElement(
- const ImagePtr &image, const QSize &size)
+ : BadgeElement(data, flags_, color_)
{
- auto element =
- (new ImageWithBackgroundLayoutElement(*this, image, size, this->color))
- ->setLink(this->getLink());
-
- return element;
}
// TEXT
diff --git a/src/messages/MessageElement.hpp b/src/messages/MessageElement.hpp
index c35f9616e..3cbb965e9 100644
--- a/src/messages/MessageElement.hpp
+++ b/src/messages/MessageElement.hpp
@@ -316,10 +316,6 @@ public:
MessageElementFlags flags_) override;
EmotePtr getEmote() const;
-protected:
- virtual MessageLayoutElement *makeImageLayoutElement(const ImagePtr &image,
- const QSize &size);
-
private:
std::unique_ptr<TextElement> textElement_;
EmotePtr emote_;
@@ -352,10 +348,6 @@ public:
const std::vector<QString> &getEmoteTooltips() const;
private:
- MessageLayoutElement *makeImageLayoutElement(
- const std::vector<ImagePtr> &image, const std::vector<QSize> &sizes,
- QSize largestSize);
-
QString getCopyString() const;
void updateTooltips();
std::vector<ImagePtr> getLoadedImages(float scale);
@@ -370,39 +362,29 @@ private:
class BadgeElement : public MessageElement
{
public:
- BadgeElement(const EmotePtr &data, MessageElementFlags flags_);
+ BadgeElement(const EmotePtr &emote, MessageElementFlags flags,
+ std::optional<QColor> bgColor = std::nullopt);
void addToContainer(MessageLayoutContainer &container,
MessageElementFlags flags_) override;
EmotePtr getEmote() const;
-protected:
- virtual MessageLayoutElement *makeImageLayoutElement(const ImagePtr &image,
- const QSize &size);
-
private:
EmotePtr emote_;
+ std::optional<QColor> bgColor_;
};
class ModBadgeElement : public BadgeElement
{
public:
ModBadgeElement(const EmotePtr &data, MessageElementFlags flags_);
-
-protected:
- MessageLayoutElement *makeImageLayoutElement(const ImagePtr &image,
- const QSize &size) override;
};
class VipBadgeElement : public BadgeElement
{
public:
VipBadgeElement(const EmotePtr &data, MessageElementFlags flags_);
-
-protected:
- MessageLayoutElement *makeImageLayoutElement(const ImagePtr &image,
- const QSize &size) override;
};
class FfzBadgeElement : public BadgeElement
@@ -410,11 +392,6 @@ class FfzBadgeElement : public BadgeElement
public:
FfzBadgeElement(const EmotePtr &data, MessageElementFlags flags_,
QColor color_);
-
-protected:
- MessageLayoutElement *makeImageLayoutElement(const ImagePtr &image,
- const QSize &size) override;
- const QColor color;
};
// contains a text, formated depending on the preferences
diff --git a/src/messages/layouts/MessageLayoutElement.cpp b/src/messages/layouts/MessageLayoutElement.cpp
index bf8aedf2c..e2619282e 100644
--- a/src/messages/layouts/MessageLayoutElement.cpp
+++ b/src/messages/layouts/MessageLayoutElement.cpp
@@ -327,9 +327,11 @@ int LayeredImageLayoutElement::getXFromIndex(size_t index)
}
PriorityImageLayoutElement::PriorityImageLayoutElement(
- MessageElement &creator, ImagePriorityOrder &&order, const QSize &size)
+ MessageElement &creator, ImagePriorityOrder &&order, const QSize &size,
+ std::optional<QColor> bgColor)
: MessageLayoutElement(creator, size)
, order_(std::move(order))
+ , bgColor_(bgColor)
{
this->trailingSpace = creator.hasTrailingSpace();
}
@@ -366,6 +368,11 @@ void PriorityImageLayoutElement::paint(QPainter &painter,
{
if (auto pixmap = imageToUse->pixmapOrLoad())
{
+ if (this->bgColor_ && this->bgColor_.value().isValid())
+ {
+ painter.fillRect(QRectF(this->getRect()),
+ this->bgColor_.value());
+ }
painter.drawPixmap(QRectF(this->getRect()), *pixmap, QRectF());
}
}
@@ -415,34 +422,6 @@ int PriorityImageLayoutElement::getXFromIndex(size_t index)
}
}
-//
-// IMAGE WITH BACKGROUND
-//
-ImageWithBackgroundLayoutElement::ImageWithBackgroundLayoutElement(
- MessageElement &creator, ImagePtr image, const QSize &size, QColor color)
- : ImageLayoutElement(creator, image, size)
- , color_(color)
-{
-}
-
-void ImageWithBackgroundLayoutElement::paint(
- QPainter &painter, const MessageColors & /*messageColors*/)
-{
- if (this->image_ == nullptr)
- {
- return;
- }
-
- auto pixmap = this->image_->pixmapOrLoad();
- if (pixmap && !this->image_->animated())
- {
- painter.fillRect(QRectF(this->getRect()), this->color_);
-
- // fourtf: make it use qreal values
- painter.drawPixmap(QRectF(this->getRect()), *pixmap, QRectF());
- }
-}
-
//
// IMAGE WITH CIRCLE BACKGROUND
//
diff --git a/src/messages/layouts/MessageLayoutElement.hpp b/src/messages/layouts/MessageLayoutElement.hpp
index 606161646..11b933a3e 100644
--- a/src/messages/layouts/MessageLayoutElement.hpp
+++ b/src/messages/layouts/MessageLayoutElement.hpp
@@ -104,7 +104,8 @@ class PriorityImageLayoutElement : public MessageLayoutElement
{
public:
PriorityImageLayoutElement(MessageElement &creator,
- ImagePriorityOrder &&order, const QSize &size);
+ ImagePriorityOrder &&order, const QSize &size,
+ std::optional<QColor> bgColor = std::nullopt);
protected:
void addCopyTextToString(QString &str, uint32_t from = 0,
@@ -116,6 +117,7 @@ protected:
int getXFromIndex(size_t index) override;
const ImagePriorityOrder order_;
+ std::optional<QColor> bgColor_;
};
class LayeredImageLayoutElement : public MessageLayoutElement
@@ -138,19 +140,6 @@ protected:
std::vector<QSize> sizes_;
};
-class ImageWithBackgroundLayoutElement : public ImageLayoutElement
-{
-public:
- ImageWithBackgroundLayoutElement(MessageElement &creator, ImagePtr image,
- const QSize &size, QColor color);
-
-protected:
- void paint(QPainter &painter, const MessageColors &messageColors) override;
-
-private:
- QColor color_;
-};
-
class ImageWithCircleBackgroundLayoutElement : public ImageLayoutElement
{
public: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist:
CHANGELOG.mdwas updated, if applicableDescription
Instead of loading image pixmap data on layout, we try to do it when the image is first painted. This PR will increase the efficacy of #3915.
To demonstrate the behavior, you can do the following:
Example Screenshots
1a810b4 on left, this PR on right.Before sending message:
After sending
PogChampmessage:Note that "Loaded images" increases by 2 on the left as the broadcaster badge was also then loaded.
After refocusing channel in which message was sent:
Todo:
Fixes #3929