Limit StrokeManager use to stroke tools #1814
Conversation
This better distinguishes it from BaseManager-derived classes
| @@ -591,8 +589,6 @@ void ScribbleArea::tabletEvent(QTabletEvent *e) | |||
| if (event.eventType() == PointerEvent::Press) | |||
| { | |||
| event.accept(); | |||
| mStrokeManager->pointerPressEvent(&event); | |||
| mStrokeManager->setTabletInUse(true); | |||
There was a problem hiding this comment.
pointer events now passed to strokemanager from stroke tools, tabletinuse now managed implicitly by strokemanager
| selectionTolerance); | ||
| mCamPathMoveMode = mode; | ||
| if (mode != CameraMoveType::NONE) | ||
| mCamMoveMode = getPathMoveMode(cam, |
There was a problem hiding this comment.
using a single move mode attribute for simplicity, since movemode values for camera handles and path handles are different this is not an issue
| @@ -117,7 +118,7 @@ QCursor CameraTool::cursor() | |||
|
|
|||
| QImage moveTypeImage; | |||
| QPoint offset = QPoint(6, 6); | |||
| switch(moveMode()) | |||
There was a problem hiding this comment.
move mode now updated in response to pointer events
| LayerCamera* layerCam = mEditor->layers()->getCameraLayerBelow(mEditor->currentLayerIndex()); | ||
| Q_ASSERT(layerCam); | ||
| mode = mEditor->overlays()->getMoveModeForPoint(getCurrentPoint(), layerCam->getViewAtFrame(mEditor->currentFrame())); | ||
| mPerspMode = mode; |
There was a problem hiding this comment.
perspmode now updated in pointer event handlers
| @@ -45,7 +45,6 @@ QCursor SelectTool::cursor() | |||
| // Don't update cursor while we're moving the selection | |||
| if (mScribbleArea->isPointerInUse()) { return QCursor(mCursorPixmap); } | |||
|
|
|||
| mEditor->select()->setMoveModeForAnchorInRange(getCurrentPoint()); | |||
There was a problem hiding this comment.
Already called in pointer event handlers, no need to do it again here
| QPoint pos() const; | ||
| * Returns the QPointF of the device, in canvas coordinates | ||
| */ | ||
| QPointF canvasPos() const; |
There was a problem hiding this comment.
| QPointF canvasPos() const; | |
| const QPointF& canvasPos() const; |
There was a problem hiding this comment.
A much appreciated decoupling that I have thought about doing so many times.
Excellent that you got around doing it!
I've looked through the code and it looks good to me, well aside from some suggestions about reference access but that's minor stuff.
My reason for neither approving nor requesting changes is that I'd like to hear your thoughts about the PointerEvent part that I mentioned before concluding my review. Is it more important that we're decoupled from ViewManager but have higher potential for creating mapping bugs when initializing PointerEvent, than either introducing a interface that sets the expectation for how the points are mapped or depend on ViewManager explicitly? 🤔
Speaking of testing the changes myself, I've tried to use all the different tools with layer combinations and it works as expected, good job 👍
While looking at the little glitch I discovered when reviewing #1806, I realised that there are quite a few components that indirectly rely on StrokeManager despite not even dealing with strokes and not requiring any StrokeManager functionality, in particular the non-stroke tools. In many of those cases, StrokeManager is used from somewhere within pointer event handling code to obtain the current cursor position even though that position is readily available from the event object.
This PR replaces those StrokeManager usages and moves StrokeManager instance from ScribbleArea to StrokeTool (where it is actually needed). This way:
event->canvasPos()vsgetCurrentPoint()) as there is one less layer of indirectionAdditional changes:
Although the number of changes in this PR is relatively higher, many of them are fairly trivial. After submitting the PR, I’ll leave some comments on the few changes that might be less obvious.