Changeset View
Standalone View
lib/documentview/documentview.cpp
| Show First 20 Lines • Show All 93 Lines • ▼ Show 20 Line(s) | 86 | { | |||
|---|---|---|---|---|---|
| 94 | LoadingIndicator* mLoadingIndicator; | 94 | LoadingIndicator* mLoadingIndicator; | ||
| 95 | 95 | | |||
| 96 | QScopedPointer<AbstractDocumentViewAdapter> mAdapter; | 96 | QScopedPointer<AbstractDocumentViewAdapter> mAdapter; | ||
| 97 | QList<qreal> mZoomSnapValues; | 97 | QList<qreal> mZoomSnapValues; | ||
| 98 | Document::Ptr mDocument; | 98 | Document::Ptr mDocument; | ||
| 99 | DocumentView::Setup mSetup; | 99 | DocumentView::Setup mSetup; | ||
| 100 | bool mCurrent; | 100 | bool mCurrent; | ||
| 101 | bool mCompareMode; | 101 | bool mCompareMode; | ||
| 102 | bool mEraseBorders; | | |||
| 103 | int controlWheelAccumulatedDelta; | 102 | int controlWheelAccumulatedDelta; | ||
| 104 | 103 | | |||
| 105 | void setCurrentAdapter(AbstractDocumentViewAdapter* adapter) | 104 | void setCurrentAdapter(AbstractDocumentViewAdapter* adapter) | ||
| 106 | { | 105 | { | ||
| 107 | Q_ASSERT(adapter); | 106 | Q_ASSERT(adapter); | ||
| 108 | mAdapter.reset(adapter); | 107 | mAdapter.reset(adapter); | ||
| 109 | 108 | | |||
| 110 | adapter->widget()->setParentItem(q); | 109 | adapter->widget()->setParentItem(q); | ||
| ▲ Show 20 Lines • Show All 230 Lines • ▼ Show 20 Line(s) | 338 | { | |||
| 341 | setFlag(ItemIsSelectable); | 340 | setFlag(ItemIsSelectable); | ||
| 342 | setFlag(ItemClipsChildrenToShape); | 341 | setFlag(ItemClipsChildrenToShape); | ||
| 343 | 342 | | |||
| 344 | d->q = this; | 343 | d->q = this; | ||
| 345 | d->mLoadingIndicator = 0; | 344 | d->mLoadingIndicator = 0; | ||
| 346 | d->mBirdEyeView = 0; | 345 | d->mBirdEyeView = 0; | ||
| 347 | d->mCurrent = false; | 346 | d->mCurrent = false; | ||
| 348 | d->mCompareMode = false; | 347 | d->mCompareMode = false; | ||
| 349 | d->mEraseBorders = false; | | |||
| 350 | d->controlWheelAccumulatedDelta = 0; | 348 | d->controlWheelAccumulatedDelta = 0; | ||
| 351 | 349 | | |||
| 352 | setOpacity(0); | 350 | setOpacity(0); | ||
| 353 | 351 | | |||
| 354 | scene->addItem(this); | 352 | scene->addItem(this); | ||
| 355 | 353 | | |||
| 356 | d->setupHud(); | 354 | d->setupHud(); | ||
| 357 | d->setCurrentAdapter(new EmptyAdapter); | 355 | d->setCurrentAdapter(new EmptyAdapter); | ||
| 358 | } | 356 | } | ||
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 | 357 | | |||
| 360 | DocumentView::~DocumentView() | 358 | DocumentView::~DocumentView() | ||
| 361 | { | 359 | { | ||
| 362 | delete d; | 360 | delete d; | ||
| 363 | } | 361 | } | ||
| 364 | 362 | | |||
| 365 | void DocumentView::createAdapterForDocument() | 363 | void DocumentView::createAdapterForDocument() | ||
| 366 | { | 364 | { | ||
| ▲ Show 20 Lines • Show All 255 Lines • ▼ Show 20 Line(s) | 618 | { | |||
| 622 | // zooming out with Ctrl + Right button | 620 | // zooming out with Ctrl + Right button | ||
| 623 | if (event->modifiers() != Qt::ControlModifier) { | 621 | if (event->modifiers() != Qt::ControlModifier) { | ||
| 624 | contextMenuRequested(); | 622 | contextMenuRequested(); | ||
| 625 | } | 623 | } | ||
| 626 | } | 624 | } | ||
| 627 | 625 | | |||
| 628 | void DocumentView::paint(QPainter* painter, const QStyleOptionGraphicsItem* /*option*/, QWidget* /*widget*/) | 626 | void DocumentView::paint(QPainter* painter, const QStyleOptionGraphicsItem* /*option*/, QWidget* /*widget*/) | ||
| 629 | { | 627 | { | ||
| 630 | QRectF visibleRect = mapRectFromItem(d->mAdapter->widget(), d->mAdapter->visibleDocumentRect()); | 628 | // This effectively fills the background with the default brush, otherwise it's transparent and other | ||
| 631 | if (d->mEraseBorders) { | 629 | // DocumentViews could be visible underneath (bad for smooth fade transitions) | ||
| 632 | QRegion borders = QRegion(boundingRect().toRect()) | 630 | painter->eraseRect(rect()); | ||
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 | 631 | | |||
| 632 | QRectF visibleRect = mapRectFromItem(d->mAdapter->widget(), d->mAdapter->visibleDocumentRect()); | ||||
rkflx: Why not move this inside the `if`? | |||||
| 639 | if (d->mCompareMode && d->mCurrent) { | 633 | if (d->mCompareMode && d->mCurrent) { | ||
| 640 | painter->save(); | 634 | painter->save(); | ||
| 641 | painter->setBrush(Qt::NoBrush); | 635 | painter->setBrush(Qt::NoBrush); | ||
| 642 | painter->setPen(QPen(palette().highlight().color(), 2)); | 636 | painter->setPen(QPen(palette().highlight().color(), 2)); | ||
| 643 | painter->setRenderHint(QPainter::Antialiasing); | 637 | painter->setRenderHint(QPainter::Antialiasing); | ||
| 644 | QRectF selectionRect = visibleRect.adjusted(-2, -2, 2, 2); | 638 | QRectF selectionRect = visibleRect.adjusted(-2, -2, 2, 2); | ||
| 645 | painter->drawRoundedRect(selectionRect, 3, 3); | 639 | painter->drawRoundedRect(selectionRect, 3, 3); | ||
| 646 | painter->restore(); | 640 | painter->restore(); | ||
| ▲ Show 20 Lines • Show All 149 Lines • ▼ Show 20 Line(s) | 789 | { | |||
| 796 | return d->mSortKey; | 790 | return d->mSortKey; | ||
| 797 | } | 791 | } | ||
| 798 | 792 | | |||
| 799 | void DocumentView::setSortKey(int sortKey) | 793 | void DocumentView::setSortKey(int sortKey) | ||
| 800 | { | 794 | { | ||
| 801 | d->mSortKey = sortKey; | 795 | d->mSortKey = sortKey; | ||
| 802 | } | 796 | } | ||
| 803 | 797 | | |||
| 804 | void DocumentView::setEraseBorders(bool value) | | |||
| 805 | { | | |||
| 806 | d->mEraseBorders = value; | | |||
| 807 | } | | |||
| 808 | | ||||
| 809 | void DocumentView::hideAndDeleteLater() | 798 | void DocumentView::hideAndDeleteLater() | ||
| 810 | { | 799 | { | ||
| 811 | hide(); | 800 | hide(); | ||
| 812 | deleteLater(); | 801 | deleteLater(); | ||
| 813 | } | 802 | } | ||
| 814 | 803 | | |||
| 815 | } // namespace | 804 | } // namespace | ||
Why not use setGraphicsEffectOpacity here too?