Differential D13901 Diff 53070 lib/documentview/documentview.cpp

Changeset View

Standalone View

View Options

lib/documentview/documentview.cpp

Show All 36 Lines
37#include <QPropertyAnimation> 37​#include <QPropertyAnimation>
38#include <QPointer> 38​#include <QPointer>
39#include <QDebug> 39​#include <QDebug>
40#include <QIcon> 40​#include <QIcon>
41#include <QUrl> 41​#include <QUrl>
42#include <QDrag> 42​#include <QDrag>
43#include <QMimeData> 43​#include <QMimeData>
44#include <QStyleHints> 44​#include <QStyleHints>
45​#include <QGestureEvent>
45 46​
ngrahamUnsubmitted
Not Done

Unnecessary whitespace change

ngraham: Unnecessary whitespace change
46// KDE 47​// KDE
47#include <KLocalizedString> 48​#include <KLocalizedString>
48#include <KFileItem> 49​#include <KFileItem>
49#include <KUrlMimeData> 50​#include <KUrlMimeData>
50 51​
ngrahamUnsubmitted
Not Done

Unnecessary whitespace change

ngraham: Unnecessary whitespace change
51// Local 52​// Local
52#include <lib/document/document.h> 53​#include <lib/document/document.h>
53#include <lib/document/documentfactory.h> 54​#include <lib/document/documentfactory.h>
54#include <lib/documentview/abstractrasterimageviewtool.h> 55​#include <lib/documentview/abstractrasterimageviewtool.h>
55#include <lib/documentview/birdeyeview.h> 56​#include <lib/documentview/birdeyeview.h>
56#include <lib/documentview/loadingindicator.h> 57​#include <lib/documentview/loadingindicator.h>
57#include <lib/documentview/messageviewadapter.h> 58​#include <lib/documentview/messageviewadapter.h>
58#include <lib/documentview/rasterimageview.h> 59​#include <lib/documentview/rasterimageview.h>
59#include <lib/documentview/rasterimageviewadapter.h> 60​#include <lib/documentview/rasterimageviewadapter.h>
60#include <lib/documentview/svgviewadapter.h> 61​#include <lib/documentview/svgviewadapter.h>
61#include <lib/documentview/videoviewadapter.h> 62​#include <lib/documentview/videoviewadapter.h>
62#include <lib/hud/hudbutton.h> 63​#include <lib/hud/hudbutton.h>
63#include <lib/hud/hudwidget.h> 64​#include <lib/hud/hudwidget.h>
64#include <lib/graphicswidgetfloater.h> 65​#include <lib/graphicswidgetfloater.h>
65#include <lib/gvdebug.h> 66​#include <lib/gvdebug.h>
66#include <lib/gwenviewconfig.h> 67​#include <lib/gwenviewconfig.h>
67#include <lib/mimetypeutils.h> 68​#include <lib/mimetypeutils.h>
68#include <lib/urlutils.h> 69​#include <lib/urlutils.h>
69#include <lib/thumbnailview/dragpixmapgenerator.h> 70​#include <lib/thumbnailview/dragpixmapgenerator.h>
70#include <lib/thumbnailprovider/thumbnailprovider.h> 71​#include <lib/thumbnailprovider/thumbnailprovider.h>
72​#include <lib/touch/touch.h>
73​#include <transformimageoperation.h>
74​#include <touch/touch_helper.h>
71 75​
72namespace Gwenview 76​namespace Gwenview
73{ 77​{
74 78​
75#undef ENABLE_LOG 79​#undef ENABLE_LOG
76#undef LOG 80​#undef LOG
77//#define ENABLE_LOG 81​//#define ENABLE_LOG
78#ifdef ENABLE_LOG 82​#ifdef ENABLE_LOG
Show All 18 Lines99{
97 int mSortKey; // Used to sort views when displayed in compare mode 101​ int mSortKey; // Used to sort views when displayed in compare mode
98 HudWidget* mHud; 102​ HudWidget* mHud;
99 BirdEyeView* mBirdEyeView; 103​ BirdEyeView* mBirdEyeView;
100 QPointer<QPropertyAnimation> mMoveAnimation; 104​ QPointer<QPropertyAnimation> mMoveAnimation;
101 QPointer<QPropertyAnimation> mFadeAnimation; 105​ QPointer<QPropertyAnimation> mFadeAnimation;
102 QGraphicsOpacityEffect* mOpacityEffect; 106​ QGraphicsOpacityEffect* mOpacityEffect;
103 107​
104 LoadingIndicator* mLoadingIndicator; 108​ LoadingIndicator* mLoadingIndicator;
105 109​
ngrahamUnsubmitted
Not Done

Unnecessary whitespace change

ngraham: Unnecessary whitespace change
106 QScopedPointer<AbstractDocumentViewAdapter> mAdapter; 110​ QScopedPointer<AbstractDocumentViewAdapter> mAdapter;
107 QList<qreal> mZoomSnapValues; 111​ QList<qreal> mZoomSnapValues;
108 Document::Ptr mDocument; 112​ Document::Ptr mDocument;
109 DocumentView::Setup mSetup; 113​ DocumentView::Setup mSetup;
110 bool mCurrent; 114​ bool mCurrent;
111 bool mCompareMode; 115​ bool mCompareMode;
112 int controlWheelAccumulatedDelta; 116​ int controlWheelAccumulatedDelta;
113 117​
114 QPointF mDragStartPosition; 118​ QPointF mDragStartPosition;
115 QPointer<ThumbnailProvider> mDragThumbnailProvider; 119​ QPointer<ThumbnailProvider> mDragThumbnailProvider;
116 QPointer<QDrag> mDrag; 120​ QPointer<QDrag> mDrag;
117 121​
122​ Touch* mTouch;
123​
rkflxUnsubmitted
Not Done

It's Tap, not Tab (applies everywhere).

rkflx: It's `Tap`, not `Tab` (applies everywhere).
118 void setCurrentAdapter(AbstractDocumentViewAdapter* adapter) 124​ void setCurrentAdapter(AbstractDocumentViewAdapter* adapter)
119 { 125​ {
120 Q_ASSERT(adapter); 126​ Q_ASSERT(adapter);
121 mAdapter.reset(adapter); 127​ mAdapter.reset(adapter);
122 128​
123 adapter->widget()->setParentItem(q); 129​ adapter->widget()->setParentItem(q);
rkflxUnsubmitted
Not Done

Member variables should be prefixed with a lowercase m.

Also, some of the names are too generic, e.g. timestamp in the context of DocumentView is not descriptive enough.

That said, I wonder why all of those variables are really necessary. Is there no Qt mechanism to differentiate between different gesture and to get at timestamps or positions of gestures?

rkflx: Member variables should be prefixed with a lowercase `m`. Also, some of the names are too…
124 resizeAdapterWidget(); 130​ resizeAdapterWidget();
125 131​
126 if (adapter->canZoom()) { 132​ if (adapter->canZoom()) {
127 QObject::connect(adapter, &AbstractDocumentViewAdapter::zoomChanged, 133​ QObject::connect(adapter, &AbstractDocumentViewAdapter::zoomChanged,
128 q, &DocumentView::slotZoomChanged); 134​ q, &DocumentView::slotZoomChanged);
129 QObject::connect(adapter, &AbstractDocumentViewAdapter::zoomInRequested, 135​ QObject::connect(adapter, &AbstractDocumentViewAdapter::zoomInRequested,
130 q, &DocumentView::zoomIn); 136​ q, &DocumentView::zoomIn);
131 QObject::connect(adapter, &AbstractDocumentViewAdapter::zoomOutRequested, 137​ QObject::connect(adapter, &AbstractDocumentViewAdapter::zoomOutRequested,
▲ Show 20 Lines • Show All 311 Lines • ▼ Show 20 Line(s)443{
443 d->mLoadingIndicator = nullptr; 449​ d->mLoadingIndicator = nullptr;
444 d->mBirdEyeView = nullptr; 450​ d->mBirdEyeView = nullptr;
445 d->mCurrent = false; 451​ d->mCurrent = false;
446 d->mCompareMode = false; 452​ d->mCompareMode = false;
447 d->controlWheelAccumulatedDelta = 0; 453​ d->controlWheelAccumulatedDelta = 0;
448 d->mDragStartPosition = QPointF(0, 0); 454​ d->mDragStartPosition = QPointF(0, 0);
449 d->mDrag = nullptr; 455​ d->mDrag = nullptr;
450 456​
457​ d->mTouch = new Touch(this);
rkflxUnsubmitted
Not Done

Remove extra space before (.

rkflx: Remove extra space before `(`.
458​ setAcceptTouchEvents (true);
459​
451 // We use an opacity effect instead of using the opacity property directly, because the latter operates at 460​ // We use an opacity effect instead of using the opacity property directly, because the latter operates at
452 // the painter level, which means if you draw multiple layers in paint(), all layers get the specified 461​ // the painter level, which means if you draw multiple layers in paint(), all layers get the specified
453 // opacity, resulting in all layers being visible when 0 < opacity < 1. 462​ // opacity, resulting in all layers being visible when 0 < opacity < 1.
ngrahamUnsubmitted
Not Done

If the reason why this code is commented out is because the features haven't been implemented yet, please add a comment indicating this.

ngraham: If the reason why this code is commented out is because the features haven't been implemented…
454 // QGraphicsEffects on the other hand, operate after all painting is done, therefore 'flattening' all layers. 463​ // QGraphicsEffects on the other hand, operate after all painting is done, therefore 'flattening' all layers.
455 // This is important for fade effects, where we don't want any background layers visible during the fade. 464​ // This is important for fade effects, where we don't want any background layers visible during the fade.
456 d->mOpacityEffect = new QGraphicsOpacityEffect(this); 465​ d->mOpacityEffect = new QGraphicsOpacityEffect(this);
457 d->mOpacityEffect->setOpacity(0); 466​ d->mOpacityEffect->setOpacity(0);
458 setGraphicsEffect(d->mOpacityEffect); 467​ setGraphicsEffect(d->mOpacityEffect);
459 468​
460 scene->addItem(this); 469​ scene->addItem(this);
461 470​
462 d->setupHud(); 471​ d->setupHud();
463 d->setCurrentAdapter(new EmptyAdapter); 472​ d->setCurrentAdapter(new EmptyAdapter);
464 473​
465 setAcceptDrops(true); 474​ setAcceptDrops(true);
466 475​
467 connect(DocumentFactory::instance(), &DocumentFactory::documentChanged, this, [this]() { 476​ connect(DocumentFactory::instance(), &DocumentFactory::documentChanged, this, [this]() {
468 d->updateCaption(); 477​ d->updateCaption();
469 }); 478​ });
470} 479​}
471 480​
472DocumentView::~DocumentView() 481​DocumentView::~DocumentView()
473{ 482​{
483​ delete d->mTouch;
474 delete d->mDragThumbnailProvider; 484​ delete d->mDragThumbnailProvider;
475 delete d->mDrag; 485​ delete d->mDrag;
476 delete d; 486​ delete d;
477} 487​}
478 488​
479void DocumentView::createAdapterForDocument() 489​void DocumentView::createAdapterForDocument()
480{ 490​{
481 const MimeTypeUtils::Kind documentKind = d->mDocument->kind(); 491​ const MimeTypeUtils::Kind documentKind = d->mDocument->kind();
▲ Show 20 Lines • Show All 223 Lines • ▼ Show 20 Line(s)714{
705 d->setZoom(zoom); 715​ d->setZoom(zoom);
706} 716​}
707 717​
708qreal DocumentView::zoom() const 718​qreal DocumentView::zoom() const
709{ 719​{
710 return d->mAdapter->zoom(); 720​ return d->mAdapter->zoom();
711} 721​}
712 722​
723​bool DocumentView::event(QEvent* event)
rkflxUnsubmitted
Not Done

I did not review event, gestureEvent and the corresponding functions in ThumbnailView in detail yet, but some general comments:

  • For a general event function, there is way too much gesture code in there. This should perhaps be split up a bit (see Qt's gestures example, 9d90a08ef5e2, or 618072f8a2ea).
  • Why is there so much code? Is there no support in Qt to detect that kind of gestures, e.g. any of QGesture's children like QTapAndHoldGesture, QPanGesture etc.? Would it make sense to subclass QGestureRecognizer if not?
  • Some parts could use more comments to make the purpose of some of the code easier to understand.
rkflx: I did not review `event`, `gestureEvent` and the corresponding functions in `ThumbnailView` in…
724​{
725​ //we need to catch all touch events, otherwise, Qt makes mouse clicks from touch events
726​ switch (event->type()) {
727​ case QEvent::TouchBegin: {
ngrahamUnsubmitted
Not Done

Coding style: opening braces in if statements go on the same line (do the same below, too).

ngraham: Coding style: opening braces in `if` statements go on the same line (do the same below, too).
728​ //Move mouse cursor to touch point, needed to fade in thumbnailbar on the top screen
729​ const QPoint pos = Touch_Helper::simpleTouchPosition(event);
730​ d->mTouch->touchToMouseMove(pos, event, Qt::NoButton);
ngrahamUnsubmitted
Not Done

Coding style: add spaces around the = (do the same below, too).

ngraham: Coding style: add spaces around the `=` (do the same below, too).
731​ return true;
732​ }
733​ case QEvent::TouchEnd:
734​ return true;
735​ case QEvent::TouchUpdate: {
736​ QTouchEvent* touchEvent = static_cast<QTouchEvent*>(event);
737​ //because we suppress the making of mouse event through Qt, we need to make our own one finger panning
738​ //but only if no TapHoldandMovingGesture is active (Drag and Drop action)
739​ if (touchEvent->touchPoints().size() == 1 && !d->mTouch->getTapHoldandMovingGestureActive()) {
740​ const QPointF delta = touchEvent->touchPoints().first().lastPos() - touchEvent->touchPoints().first().pos();
ngrahamUnsubmitted
Not Done

Coding style: this line is very long and a bit difficult to read. Might be better formatted with variables, like so:

const QPointF start = touchevent->touchPoints().at(0).startPos();
const QPointF end = touchevent->touchPoints().at(0).pos();
QPoint diff = start - end;
ngraham: Coding style: this line is very long and a bit difficult to read. Might be better formatted…
741​ d->mAdapter->setScrollPos(d->mAdapter->scrollPos() + delta);
742​ }
743​ return true;
744​ }
745​ case QEvent::Gesture: {
746​ gestureEvent(static_cast<QGestureEvent*>(event));
ngrahamUnsubmitted
Not Done

Coding style: comments should be in English. @rkflx (or many others) can probably provide an English translation for you.

ngraham: Coding style: comments should be in English. @rkflx (or many others) can probably provide an…
747​ return true;
ngrahamUnsubmitted
Not Done

Coding style: space after the <

ngraham: Coding style: space after the `<`
748​ }
749​ default:
ngrahamUnsubmitted
Not Done

Coding style: indent this line (do the same below, too).

ngraham: Coding style: indent this line (do the same below, too).
750​ break;
751​ }
752​
753​ return QGraphicsWidget::event(event);
754​}
755​
756​void DocumentView::gestureEvent(QGestureEvent* event)
757​{
758​ if (d->mTouch->checkTwoFingerTapGesture(event)) {
759​ contextMenuRequested();
760​ }
761​
ngrahamUnsubmitted
Not Done

Coding style: unnecessary whitespace

ngraham: Coding style: unnecessary whitespace
762​ if (d->mTouch->checkTapGesture(event)) {
763​ QPoint pos = d->mTouch->positionGesture(event);
764​ d->mTouch->touchToMouseClick(pos, event->widget());
765​ }
766​
767​ if (d->mTouch->checkTapHoldAndMovingGesture(event, this)) {
768​ d->startDragIfSensible();
769​ }
770​
771​ if (QGesture* gesture = event->gesture(d->mTouch->getDoubleTapGesture())) {
772​ event->accept();
773​ if (gesture->state() == Qt::GestureFinished) {
ngrahamUnsubmitted
Not Done

Coding style: insufficient indentation for this level and everything below it

ngraham: Coding style: insufficient indentation for this level and everything below it
774​ d->mAdapter->toggleFullScreenRequested();
775​ }
776​ }
777​
778​ if (QGesture* gesture = event->gesture(d->mTouch->getOneAndTwoFingerSwipeGesture())) {
779​ event->accept();
780​ oneAndTwoFingerSwipeGestureTriggered(gesture);
781​ }
782​
783​ if (QGesture* gesture = event->gesture(d->mTouch->getTwoFingerPanGesture())) {
784​ event->accept();
785​ d->mTouch->setLastPanGestureState(event);
786​ if (gesture->state() == Qt::GestureUpdated) {
787​ const QPoint diff = gesture->property("delta").toPoint();
788​ d->mAdapter->setScrollPos(d->mAdapter->scrollPos() + diff);
ngrahamUnsubmitted
Not Done

coding style: add whitespace around the ,

ngraham: coding style: add whitespace around the `,`
789​ }
790​ }
791​
792​ if (d->mTouch->checkPinchGesture(event)) {
793​ const QPinchGesture* pinch = static_cast<QPinchGesture*>(event->gesture(Qt::PinchGesture));
794​ const qreal sensitivityModifier = 0.85;
795​ const qreal rotationThreshold = 40;
796​ const qreal newZoom = d->mTouch->getZoomFromPinchGesture(event, sensitivityModifier, zoom());
797​ if (newZoom >= 0.0 && d->mAdapter->canZoom()) {
798​ const QPointF zoomCenter = mapFromScene(event->mapToGraphicsScene(pinch->centerPoint()));
799​ d->setZoom(newZoom, zoomCenter);
800​ }
801​
802​ const qreal rotation = d->mTouch->getRotationFromPinchGesture(event, rotationThreshold);
803​ if (rotation > 0.0) {
804​ TransformImageOperation* op = new TransformImageOperation(ROT_90);
805​ op->applyToDocument(d->mDocument);
806​ } else if (rotation < 0.0) {
807​ TransformImageOperation* op = new TransformImageOperation(ROT_270);
808​ op->applyToDocument(d->mDocument);
809​ }
810​ }
811​}
812​
813​
814​
815​void DocumentView::oneAndTwoFingerSwipeGestureTriggered(QGesture* gesture)
816​{
817​ //only change image if the viewport on the right or left border of the zoomed-in image
818​ if (gesture->state() == Qt::GestureFinished) {
819​ if (gesture->property("right").toBool()) {
820​ const QPoint scrollPos = d->mAdapter->scrollPos().toPoint();
821​ if (scrollPos.x() <= 1) {
822​ d->mAdapter->previousImageRequested();
823​ }
824​ }
825​ if (gesture->property("left").toBool()) {
ngrahamUnsubmitted
Not Done

Should probably be sensitivityModifier

ngraham: Should probably be `sensitivityModifier`
826​ const QPoint scrollPos = d->mAdapter->scrollPos().toPoint();
827​ const int width = d->mAdapter->document()->width() * d->mAdapter->zoom();
828​ const QRect visibleRect = d->mAdapter->visibleDocumentRect().toRect();
829​ const int x = scrollPos.x() + visibleRect.width();
830​ if (x >= (width - 1)) {
831​ d->mAdapter->nextImageRequested();
832​ }
833​ }
834​ }
835​}
836​
713void DocumentView::resizeEvent(QGraphicsSceneResizeEvent *event) 837​void DocumentView::resizeEvent(QGraphicsSceneResizeEvent *event)
714{ 838​{
715 d->resizeAdapterWidget(); 839​ d->resizeAdapterWidget();
716 d->updateZoomSnapValues(); 840​ d->updateZoomSnapValues();
717 QGraphicsWidget::resizeEvent(event); 841​ QGraphicsWidget::resizeEvent(event);
718} 842​}
719 843​
720void DocumentView::mousePressEvent(QGraphicsSceneMouseEvent* event) 844​void DocumentView::mousePressEvent(QGraphicsSceneMouseEvent* event)
▲ Show 20 Lines • Show All 220 Lines • ▼ Show 20 Line(s)1061 if (event->type() == QEvent::GraphicsSceneMousePress) {
941 } 1065​ }
942 QMetaObject::invokeMethod(this, "emitFocused", Qt::QueuedConnection); 1066​ QMetaObject::invokeMethod(this, "emitFocused", Qt::QueuedConnection);
943 } else if (event->type() == QEvent::GraphicsSceneHoverMove) { 1067​ } else if (event->type() == QEvent::GraphicsSceneHoverMove) {
944 if (d->mBirdEyeView) { 1068​ if (d->mBirdEyeView) {
945 d->mBirdEyeView->onMouseMoved(); 1069​ d->mBirdEyeView->onMouseMoved();
946 } 1070​ }
947 } else if (event->type() == QEvent::GraphicsSceneMouseMove) { 1071​ } else if (event->type() == QEvent::GraphicsSceneMouseMove) {
948 const QGraphicsSceneMouseEvent* mouseEvent = static_cast<QGraphicsSceneMouseEvent*>(event); 1072​ const QGraphicsSceneMouseEvent* mouseEvent = static_cast<QGraphicsSceneMouseEvent*>(event);
1073​ //in some older version of Qt, Qt synthesize a mouse event from the touch event
rkflxUnsubmitted
Not Done

Space before { missing.

rkflx: Space before `{` missing.
ngrahamUnsubmitted
Not Done

Which older versions? If it's older than 5.9, we don't need this.

ngraham: Which older versions? If it's older than 5.9, we don't need this.
steffenhAuthorUnsubmitted
Not Done

can we keep this for the moment, because I need it on my working system (OpenSUSE Leap, Qt 5.9.4, KDE 5.45) ?

steffenh: can we keep this for the moment, because I need it on my working system (OpenSUSE Leap, Qt 5.9.
ngrahamUnsubmitted
Not Done

OK.

ngraham: OK.
1074​ //we need to suppress this.
1075​ //I need this for my working system (OpenSUSE Leap 15.0, Qt 5.9.4)
1076​ if (mouseEvent->source() == Qt::MouseEventSynthesizedByQt) {
1077​ return true;
1078​ }
949 const qreal dragDistance = (mouseEvent->pos() - d->mDragStartPosition).manhattanLength(); 1079​ const qreal dragDistance = (mouseEvent->pos() - d->mDragStartPosition).manhattanLength();
950 const qreal minDistanceToStartDrag = QGuiApplication::styleHints()->startDragDistance(); 1080​ const qreal minDistanceToStartDrag = QGuiApplication::styleHints()->startDragDistance();
951 if (!d->canPan() && dragDistance >= minDistanceToStartDrag) { 1081​ if (!d->canPan() && dragDistance >= minDistanceToStartDrag) {
rkflxUnsubmitted
Not Done

Is this still needed given the check above?

rkflx: Is this still needed given the check above?
952 d->startDragIfSensible(); 1082​ d->startDragIfSensible();
953 } 1083​ }
954 } 1084​ }
955 return false; 1085​ return false;
956} 1086​}
957 1087​
958AbstractRasterImageViewTool* DocumentView::currentTool() const 1088​AbstractRasterImageViewTool* DocumentView::currentTool() const
959{ 1089​{
▲ Show 20 Lines • Show All 63 Lines • Show Last 20 Lines