feat(appearance): reimplement snow#194
Conversation
e893f22 to
b220656
Compare
Signed-off-by: kaeeraa <kaeeraa@nebula-nook.ru>
so5iso4ka
left a comment
so5iso4ka
left a comment
There was a problem hiding this comment.
For some or all files: fix header includes and use correct license headers.
| QAbstractItemView::currentChanged(current, previous); | ||
| // TODO: for accessibility support, implement+register a factory, steal QAccessibleTable from Qt and return an instance of it for | ||
| // InstanceView. | ||
| #ifndef QT_NO_ACCESSIBILITY |
| void generateSnow(); | ||
| void reflowSnowflakes(); | ||
| void drawSnow(QPainter& painter); | ||
| static QColor getSnowColor(); |
There was a problem hiding this comment.
Maybe we should move both of these functions to an anonymous namespace? Since they aren't directly related to InstanceView.
| return; | ||
| } | ||
|
|
||
| QTimer::singleShot(0, this, [this] { generateSnow(); }); |
There was a problem hiding this comment.
This is called from the constructor, and the QTimer documentation states: "As a special case, a QTimer with a timeout of 0 will time out as soon as possible, though the ordering between zero-timeout timers and other sources of events is unspecified." If I understand correctly, it may call generateSnow() before the widget reaches its normal size, in which case it won’t generate any snowflakes. I think it would be better to move this call to resizeEvent, based on the normal size and the missing snowflakes.
| SnowflakePack* targetPack = nullptr; | ||
| bool isGif = false; | ||
|
|
||
| for (auto* pack : packs) { |
There was a problem hiding this comment.
Unnecessary code; no snow from multiple snowpacks. It's also too heavy for frequent calls from paintEvent.
| if (m_snowVisible) { | ||
| const QSize newSize = viewport()->size(); | ||
|
|
||
| if (m_lastViewportSize.isValid()) { |
There was a problem hiding this comment.
QSize::isValid(): "Returns true if both the width and height are equal to or greater than 0; otherwise returns false." This will allow a zero size, which will result in division by zero. QSize::isEmpty() would be more appropriate here.
| } | ||
| m_ui->snowColorComboBox->setCurrentIndex(snowColorIndex); | ||
|
|
||
| applySnow(m_ui->snowBox->isChecked()); |
| APPLICATION->currentSnowChanged(visible); | ||
| } | ||
|
|
||
| void AppearanceWidget::applySnowColor(const QString& color) |
There was a problem hiding this comment.
Unnecessary wrapping of a single line.
| } | ||
| } | ||
|
|
||
| if (m_ui->snowflakePackComboBox->currentIndex() < 0 && defaultSnowflakeIndex >= 0) { |
There was a problem hiding this comment.
QComboBox::currentIndex() will not be -1 after the very first item is added on line 404.
There was a problem hiding this comment.
setMinimum(int) and setMaximum(int) do not handle cases where the minimum is greater than the maximum, which causes undefined behavior in std::clamp.
| return; | ||
| } | ||
|
|
||
| if (event->key() == Qt::Key_Tab || event->key() == Qt::Key_Backtab) { |
There was a problem hiding this comment.
Unnecessary code, this is handled in RangeSlider::event().
this sounds scary lol
imagine someone just goes to you and says this guy requested changes NOW |
4 participants
yeah, go on. steal this +2000 feature