Changeset View
Standalone View
lib/documentview/documentview.cpp
| Show All 25 Lines | |||||
| 26 | 26 | | |||
| 27 | // Qt | 27 | // Qt | ||
| 28 | #include <QApplication> | 28 | #include <QApplication> | ||
| 29 | #include <QGraphicsLinearLayout> | 29 | #include <QGraphicsLinearLayout> | ||
| 30 | #include <QGraphicsProxyWidget> | 30 | #include <QGraphicsProxyWidget> | ||
| 31 | #include <QGraphicsScene> | 31 | #include <QGraphicsScene> | ||
| 32 | #include <QGraphicsSceneMouseEvent> | 32 | #include <QGraphicsSceneMouseEvent> | ||
| 33 | #include <QGraphicsSceneWheelEvent> | 33 | #include <QGraphicsSceneWheelEvent> | ||
| 34 | #include <QGraphicsOpacityEffect> | ||||
| 34 | #include <QPainter> | 35 | #include <QPainter> | ||
| 35 | #include <QPropertyAnimation> | 36 | #include <QPropertyAnimation> | ||
| 36 | #include <QPointer> | 37 | #include <QPointer> | ||
| 37 | #include <QDebug> | 38 | #include <QDebug> | ||
| 38 | #include <QIcon> | 39 | #include <QIcon> | ||
| 39 | #include <QUrl> | 40 | #include <QUrl> | ||
| 40 | 41 | | |||
| 41 | // KDE | 42 | // KDE | ||
| ▲ Show 20 Lines • Show All 43 Lines • ▼ Show 20 Line(s) | |||||
| 85 | struct DocumentViewPrivate | 86 | struct DocumentViewPrivate | ||
| 86 | { | 87 | { | ||
| 87 | DocumentView* q; | 88 | DocumentView* q; | ||
| 88 | int mSortKey; // Used to sort views when displayed in compare mode | 89 | int mSortKey; // Used to sort views when displayed in compare mode | ||
| 89 | HudWidget* mHud; | 90 | HudWidget* mHud; | ||
| 90 | BirdEyeView* mBirdEyeView; | 91 | BirdEyeView* mBirdEyeView; | ||
| 91 | QPointer<QPropertyAnimation> mMoveAnimation; | 92 | QPointer<QPropertyAnimation> mMoveAnimation; | ||
| 92 | QPointer<QPropertyAnimation> mFadeAnimation; | 93 | QPointer<QPropertyAnimation> mFadeAnimation; | ||
| 94 | QGraphicsOpacityEffect* mOpacityEffect; | ||||
| 93 | 95 | | |||
| 94 | LoadingIndicator* mLoadingIndicator; | 96 | LoadingIndicator* mLoadingIndicator; | ||
| 95 | 97 | | |||
| 96 | QScopedPointer<AbstractDocumentViewAdapter> mAdapter; | 98 | QScopedPointer<AbstractDocumentViewAdapter> mAdapter; | ||
| 97 | QList<qreal> mZoomSnapValues; | 99 | QList<qreal> mZoomSnapValues; | ||
| 98 | Document::Ptr mDocument; | 100 | Document::Ptr mDocument; | ||
| 99 | DocumentView::Setup mSetup; | 101 | DocumentView::Setup mSetup; | ||
| 100 | bool mCurrent; | 102 | bool mCurrent; | ||
| 101 | bool mCompareMode; | 103 | bool mCompareMode; | ||
| 102 | bool mEraseBorders; | | |||
| 103 | int controlWheelAccumulatedDelta; | 104 | int controlWheelAccumulatedDelta; | ||
| 104 | 105 | | |||
| 105 | void setCurrentAdapter(AbstractDocumentViewAdapter* adapter) | 106 | void setCurrentAdapter(AbstractDocumentViewAdapter* adapter) | ||
| 106 | { | 107 | { | ||
| 107 | Q_ASSERT(adapter); | 108 | Q_ASSERT(adapter); | ||
| 108 | mAdapter.reset(adapter); | 109 | mAdapter.reset(adapter); | ||
| 109 | 110 | | |||
| 110 | adapter->widget()->setParentItem(q); | 111 | adapter->widget()->setParentItem(q); | ||
| ▲ Show 20 Lines • Show All 203 Lines • ▼ Show 20 Line(s) | 314 | { | |||
| 314 | if (mFadeAnimation.data()) { | 315 | if (mFadeAnimation.data()) { | ||
| 315 | qreal endValue = mFadeAnimation.data()->endValue().toReal(); | 316 | qreal endValue = mFadeAnimation.data()->endValue().toReal(); | ||
| 316 | if (qFuzzyCompare(value, endValue)) { | 317 | if (qFuzzyCompare(value, endValue)) { | ||
| 317 | // Same end value, don't change the actual animation | 318 | // Same end value, don't change the actual animation | ||
| 318 | return; | 319 | return; | ||
| 319 | } | 320 | } | ||
| 320 | } | 321 | } | ||
| 321 | // Create a new fade animation | 322 | // Create a new fade animation | ||
| 322 | QPropertyAnimation* anim = new QPropertyAnimation(q, "opacity"); | 323 | QPropertyAnimation* anim = new QPropertyAnimation(mOpacityEffect, "opacity"); | ||
| 323 | anim->setStartValue(q->opacity()); | 324 | anim->setStartValue(mOpacityEffect->opacity()); | ||
| 324 | anim->setEndValue(value); | 325 | anim->setEndValue(value); | ||
| 325 | if (qFuzzyCompare(value, 1)) { | 326 | if (qFuzzyCompare(value, 1)) { | ||
| 326 | QObject::connect(anim, SIGNAL(finished()), | 327 | QObject::connect(anim, SIGNAL(finished()), | ||
| 327 | q, SLOT(slotFadeInFinished())); | 328 | q, SLOT(slotFadeInFinished())); | ||
| 328 | } | 329 | } | ||
| 329 | QObject::connect(anim, SIGNAL(finished()), q, SIGNAL(isAnimatedChanged())); | 330 | QObject::connect(anim, SIGNAL(finished()), q, SIGNAL(isAnimatedChanged())); | ||
| 330 | anim->setDuration(DocumentView::AnimDuration); | 331 | anim->setDuration(DocumentView::AnimDuration); | ||
| 331 | mFadeAnimation = anim; | 332 | mFadeAnimation = anim; | ||
| Show All 9 Lines | 340 | { | |||
| 341 | setFlag(ItemIsSelectable); | 342 | setFlag(ItemIsSelectable); | ||
| 342 | setFlag(ItemClipsChildrenToShape); | 343 | setFlag(ItemClipsChildrenToShape); | ||
| 343 | 344 | | |||
| 344 | d->q = this; | 345 | d->q = this; | ||
| 345 | d->mLoadingIndicator = 0; | 346 | d->mLoadingIndicator = 0; | ||
| 346 | d->mBirdEyeView = 0; | 347 | d->mBirdEyeView = 0; | ||
| 347 | d->mCurrent = false; | 348 | d->mCurrent = false; | ||
| 348 | d->mCompareMode = false; | 349 | d->mCompareMode = false; | ||
| 349 | d->mEraseBorders = false; | | |||
| 350 | d->controlWheelAccumulatedDelta = 0; | 350 | d->controlWheelAccumulatedDelta = 0; | ||
| 351 | 351 | | |||
| 352 | setOpacity(0); | 352 | // We use an opacity effect instead of using the opacity property directly, because the latter operates at | ||
| 353 | // the painter level, which means if you draw multiple layers in paint(), all layers get the specified | ||||
| 354 | // opacity, resulting in all layers being visible when 0 < opacity < 1. | ||||
| 355 | // QGraphicsEffects on the other hand, operate after all painting is done, therefore 'flattening' all layers. | ||||
| 356 | // This is important for fade effects, where we don't want any background layers visible during the fade. | ||||
| 357 | d->mOpacityEffect = new QGraphicsOpacityEffect(this); | ||||
| 358 | d->mOpacityEffect->setOpacity(0); | ||||
rkflx: Why not use `setGraphicsEffectOpacity` here too? | |||||
Yes I suppose so. Seems a bit odd calling a local function here when we're setting things up though? I can't think of a situation is change the function without touching this code. Perhaps a better solution would be to put all this code in setGraphicsEffectOpacity, creating and applying the effect the first time it's called. huoni: Yes I suppose so. Seems a bit odd calling a local function here when we're setting things up… | |||||
My thinking was that in case later someone adds code to setGraphicsEffectOpacity, it would be easy to miss adding the code here too. Anyway, you could also just keep it as it is now. No need to overcomplicate this… Also, keeping the init code in the constructor is not too bad either. rkflx: My thinking was that in case later someone adds code to `setGraphicsEffectOpacity`, it would be… | |||||
I see the logic, but it is just a basic setter. I think calling setters in the constructor is generally not favourable. I think I will leave it as is, if that's okay :) huoni: > My thinking was that in case later someone adds code to `setGraphicsEffectOpacity`, it would… | |||||
Sure thing ;) However, I don't find linking to a Java question on SO very convincing, in particular because there are differences regarding handling of virtual vs. C++. Also, I would not call this a "setter", as it's not operating on its own private member, but calling a (arbitrary from out PoV) function on an object. Still, there are probably other reason why this is good in one situation and bad in another, so as I said, it's fine the way you landed it… rkflx: Sure thing ;)
---
However, I don't find linking to a Java question on SO very convincing, in… | |||||
| 359 | setGraphicsEffect(d->mOpacityEffect); | ||||
| 353 | 360 | | |||
| 354 | scene->addItem(this); | 361 | scene->addItem(this); | ||
| 355 | 362 | | |||
| 356 | d->setupHud(); | 363 | d->setupHud(); | ||
| 357 | d->setCurrentAdapter(new EmptyAdapter); | 364 | d->setCurrentAdapter(new EmptyAdapter); | ||
| 358 | } | 365 | } | ||
| 359 | 366 | | |||
| 360 | DocumentView::~DocumentView() | 367 | DocumentView::~DocumentView() | ||
| ▲ Show 20 Lines • Show All 261 Lines • ▼ Show 20 Line(s) | 627 | { | |||
| 622 | // zooming out with Ctrl + Right button | 629 | // zooming out with Ctrl + Right button | ||
| 623 | if (event->modifiers() != Qt::ControlModifier) { | 630 | if (event->modifiers() != Qt::ControlModifier) { | ||
| 624 | contextMenuRequested(); | 631 | contextMenuRequested(); | ||
| 625 | } | 632 | } | ||
| 626 | } | 633 | } | ||
| 627 | 634 | | |||
| 628 | void DocumentView::paint(QPainter* painter, const QStyleOptionGraphicsItem* /*option*/, QWidget* /*widget*/) | 635 | void DocumentView::paint(QPainter* painter, const QStyleOptionGraphicsItem* /*option*/, QWidget* /*widget*/) | ||
| 629 | { | 636 | { | ||
| 630 | QRectF visibleRect = mapRectFromItem(d->mAdapter->widget(), d->mAdapter->visibleDocumentRect()); | 637 | // Fill background manually, because setAutoFillBackground(true) fill with QPalette::Window, | ||
| 631 | if (d->mEraseBorders) { | 638 | // but our palettes use QPalette::Base for the background color/texture | ||
| 632 | QRegion borders = QRegion(boundingRect().toRect()) | 639 | painter->fillRect(rect(), palette().base()); | ||
eraseRect() no longer worked after implementing QGraphicsOpacityEffect because painter->background() (which is the brush eraseRect uses) was returning pure white until after the fade animation had finished. huoni: `eraseRect()` no longer worked after implementing `QGraphicsOpacityEffect` because `painter… | |||||
| 633 | - QRegion(visibleRect.toRect()); | | |||
| 634 | Q_FOREACH(const QRect& rect, borders.rects()) { | | |||
| 635 | painter->eraseRect(rect); | | |||
| 636 | } | | |||
| 637 | } | | |||
| 638 | 640 | | |||
| 641 | // Selection indicator/highlight | ||||
rkflx: Why not move this inside the `if`? | |||||
| 639 | if (d->mCompareMode && d->mCurrent) { | 642 | if (d->mCompareMode && d->mCurrent) { | ||
| 643 | QRectF visibleRect = mapRectFromItem(d->mAdapter->widget(), d->mAdapter->visibleDocumentRect()); | ||||
| 640 | painter->save(); | 644 | painter->save(); | ||
| 641 | painter->setBrush(Qt::NoBrush); | 645 | painter->setBrush(Qt::NoBrush); | ||
| 642 | painter->setPen(QPen(palette().highlight().color(), 2)); | 646 | painter->setPen(QPen(palette().highlight().color(), 2)); | ||
| 643 | painter->setRenderHint(QPainter::Antialiasing); | 647 | painter->setRenderHint(QPainter::Antialiasing); | ||
| 644 | QRectF selectionRect = visibleRect.adjusted(-2, -2, 2, 2); | 648 | QRectF selectionRect = visibleRect.adjusted(-2, -2, 2, 2); | ||
| 645 | painter->drawRoundedRect(selectionRect, 3, 3); | 649 | painter->drawRoundedRect(selectionRect, 3, 3); | ||
| 646 | painter->restore(); | 650 | painter->restore(); | ||
| 647 | } | 651 | } | ||
| ▲ Show 20 Lines • Show All 148 Lines • ▼ Show 20 Line(s) | 799 | { | |||
| 796 | return d->mSortKey; | 800 | return d->mSortKey; | ||
| 797 | } | 801 | } | ||
| 798 | 802 | | |||
| 799 | void DocumentView::setSortKey(int sortKey) | 803 | void DocumentView::setSortKey(int sortKey) | ||
| 800 | { | 804 | { | ||
| 801 | d->mSortKey = sortKey; | 805 | d->mSortKey = sortKey; | ||
| 802 | } | 806 | } | ||
| 803 | 807 | | |||
| 804 | void DocumentView::setEraseBorders(bool value) | | |||
| 805 | { | | |||
| 806 | d->mEraseBorders = value; | | |||
| 807 | } | | |||
| 808 | | ||||
| 809 | void DocumentView::hideAndDeleteLater() | 808 | void DocumentView::hideAndDeleteLater() | ||
| 810 | { | 809 | { | ||
| 811 | hide(); | 810 | hide(); | ||
| 812 | deleteLater(); | 811 | deleteLater(); | ||
| 813 | } | 812 | } | ||
| 814 | 813 | | |||
| 814 | void DocumentView::setOpacityEffectEnabled(bool enabled) | ||||
| 815 | { | ||||
| 816 | setGraphicsEffect(enabled ? d->mOpacityEffect : 0); | ||||
| 817 | } | ||||
Do you have further plans with this function? If not, the code could be written inline. rkflx: Do you have further plans with this function? If not, the code could be written inline. | |||||
My thinking was since setGraphicsEffect is generic, I wanted to make it clearer that we're dealing with the opacity. It now directly calls setGraphicsEffect(0) from DocumentViewContainer::updateLayout, and I've put a comment to explain that it's the opacity. Is this what you meant? Perhaps it would be preferred to add a function that effectivly replaces setOpacity, and acts on the graphics effect? huoni: My thinking was since `setGraphicsEffect` is generic, I wanted to make it clearer that we're… | |||||
While it is true that an appropriately named function can help document the code, adding one-liners everywhere for that exact purpose feels counter-productive to me, because it increases cognitive costs for the reader of the code. I'd say the comment is just fine in this case. rkflx: While it is true that an appropriately named function can help document the code, adding one… | |||||
After your latest update, the function call makes sense, because you actually need to access the d-pointer. rkflx: After your latest update, the function call makes sense, because you actually need to access… | |||||
| 818 | | ||||
| 815 | } // namespace | 819 | } // namespace | ||
Why not use setGraphicsEffectOpacity here too?