Fix selection rendering #4830
Fix selection rendering #4830
Conversation
|
I'm unable to reproduce the reported issue, could you provide some more reproduction instructions? |
This allows us to more closely know what we're calling & what we're consuming when painting the rect
be consistent with `given point`
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| for (auto i = this->lines_.back().startIndex; | ||
| i < this->elements_.size(); i++) | ||
| { | ||
| QPoint prevPos = this->elements_[i]->getRect().topLeft(); |
There was a problem hiding this comment.
warning: variable 'prevPos' of type 'QPoint' can be declared 'const' [misc-const-correctness]
| QPoint prevPos = this->elements_[i]->getRect().topLeft(); | |
| QPoint const prevPos = this->elements_[i]->getRect().topLeft(); |
|
|
||
| int xOffset = 0; | ||
|
|
||
| if (this->flags_.has(MessageFlag::Centered) && this->elements_.size() > 0) |
There was a problem hiding this comment.
warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
| if (this->flags_.has(MessageFlag::Centered) && this->elements_.size() > 0) | |
| if (this->flags_.has(MessageFlag::Centered) && !this->elements_.empty()) |
Additional context
/usr/include/c++/11/bits/stl_vector.h:1006: method 'vector'::empty() defined here
empty() const _GLIBCXX_NOEXCEPT
^| { | ||
| MessageLayoutElement *element = this->elements_.at(i).get(); | ||
|
|
||
| bool isCompactEmote = |
There was a problem hiding this comment.
warning: variable 'isCompactEmote' of type 'bool' can be declared 'const' [misc-const-correctness]
| bool isCompactEmote = | |
| bool const isCompactEmote = |
| int yExtra = 0; | ||
| if (isCompactEmote) | ||
| { | ||
| yExtra = (COMPACT_EMOTES_OFFSET / 2) * this->scale_; |
There was a problem hiding this comment.
warning: narrowing conversion from 'float' to 'int' [bugprone-narrowing-conversions]
yExtra = (COMPACT_EMOTES_OFFSET / 2) * this->scale_;
^| int yExtra = 0; | ||
| if (isCompactEmote) | ||
| { | ||
| yExtra = (COMPACT_EMOTES_OFFSET / 2) * this->scale_; |
There was a problem hiding this comment.
warning: result of integer division used in a floating point context; possible loss of precision [bugprone-integer-division]
yExtra = (COMPACT_EMOTES_OFFSET / 2) * this->scale_;
^Additional context
src/messages/layouts/MessageLayoutContainer.cpp:19: expanded from macro 'COMPACT_EMOTES_OFFSET'
#define COMPACT_EMOTES_OFFSET 4
^| @@ -180,7 +595,7 @@ void MessageLayoutContainer::_addElement(MessageLayoutElement *element, | |||
| element->getCreator().getFlags().hasNone( | |||
| {MessageElementFlag::TwitchEmoteImage})) | |||
| { | |||
| yOffset -= (this->margin.top * this->scale_); | |||
| yOffset -= (MARGIN.top() * this->scale_); | |||
There was a problem hiding this comment.
warning: narrowing conversion from 'float' to 'int' [bugprone-narrowing-conversions]
yOffset -= (MARGIN.top() * this->scale_);
^| { | ||
| int c = this->elements_[i]->getSelectionIndexCount(); | ||
| size_t c = this->elements_[i]->getSelectionIndexCount(); |
There was a problem hiding this comment.
warning: variable 'c' of type 'size_t' (aka 'unsigned long') can be declared 'const' [misc-const-correctness]
| size_t c = this->elements_[i]->getSelectionIndexCount(); | |
| size_t const c = this->elements_[i]->getSelectionIndexCount(); |
The rendering of selections was not aligned to the actual selection that took place for newlines at the end of messages, if they were the only part that was selected of that message. In addition to that fix, we've already refactored the MessageLayoutContainer to try to make it a little bit more sane to work with in the future. Co-authored-by: Rasmus Karlsson <rasmus.karlsson@pajlada.com>
Description
After 3265df7, the selection rendering was off. Selecting the trailing newline of a message (upwards) would render the whole last line of a message to be selected. However, when copying the text, only the trailing newline of the message would be included.
This PR refactors the rendering of the selection to be a bit more readable and to reduce
sign-comparewarnings in the code (see #4825). In addition, a special case for the newline is added to be consistent with rendering the selection of the "trailing newline" (downwards).Even though the selection is now correctly rendered, this might not be the exact behavior one would expect. Not including the trailing newline might be desired. Feel free to discuss this here and I can potentially adjust the behavior.