Add a rotate slider to the overview docker.
ClosedPublic

Authored by woltherav on Oct 11 2018, 1:29 PM.

Details

Summary

Like every other spinbox slider, rightclicking will allow input, and thus
setting to 0 will reset canvas rotation.

This is a rewrite of https://phabricator.kde.org/D2518

With this, the 4 and 6 buttons, the touch docker buttons, the view menu entries
the pop-up palette widget, the shift+space, I never want to hear again that
Krita lacks options to rotate the canvas.

Test Plan

I tested this with...

  • the slider itself
  • the shift+space modifier
  • the pop-up palette
  • 4 and 6
  • multiple documents open at once with different rotations
  • closing documents.

It does not behave unexpectedly with either of these situations.

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
woltherav created this revision.Oct 11 2018, 1:29 PM
Restricted Application added a project: Krita. · View Herald TranscriptOct 11 2018, 1:29 PM
woltherav requested review of this revision.Oct 11 2018, 1:29 PM

Krita lacks options to rotate the canvas

Two-finger touch gesture doesn't do rotation, so here you go :)

woltherav added a comment.EditedOct 11 2018, 3:21 PM

Krita lacks options to rotate the canvas

Two-finger touch gesture doesn't do rotation, so here you go :)

You know just as well as I do that that particular code is a mystery spaggetti. This was particular because people claim Krita has no view rotation at all despite the half-a-dozen different options to access it because it doesn't show up in the overview docker.

dkazakov requested changes to this revision.Oct 12 2018, 9:10 AM
dkazakov added a reviewer: scottpetrovic.
dkazakov added a subscriber: scottpetrovic.

Hi, @woltherav!

The patch basically works. The sender() thing (see inline) is actually the only serious blocker I see.

I would also personally prefer that the rotation slider would look the zoom slider above. Right now the two sliders of different styles look a bit clumsy :) It would be nice at least make them at least both aligned the same way.

But GUI thing is only my opinion, the final word should be on @scottpetrovic (adding him as a direct reviewer).

plugins/dockers/overview/overviewdocker_dock.cpp
106

Erm... why this check is needed?

If it is about avoiding cycling updates via OverviewDockerDock::updateSlider(), then it is better to use KisSignalsBlocker to block slider's signals for the update time. Usage of sender() method is discouraged by Qt's documentation, because it can work a bit unexpectedly sometimes...

This revision now requires changes to proceed.Oct 12 2018, 9:10 AM

I haven't tested out the patch but I think the UI is fine. If we get more controls on it we might want to think about the UI more. For now this seems fine.

Hi, @woltherav!

I think the sender() problem is the only thing that blocks the patch. Please refactor it to use KisSignalsBlocker and push :)

woltherav updated this revision to Diff 51874.Feb 16 2019, 4:01 PM

Okay, I removed the signal part, but I felt a desire to add a mirror view button.

To get this one to update properly, I needed to get the kis_canvas_controller to send out updates, because none of the KoCanvasController signals were able to notify about mirroring state when the canvas is fully zoomed out. The mirror button in the pop-up palette suffers from the same issue.

edit: actually, this seems to conflict with the mirror button on the toolbar(if that's activated)... :|

woltherav updated this revision to Diff 51904.Feb 17 2019, 3:42 PM

Ok, now using the action directly. This fixed the issues I found AND required less code :)

rempt added a subscriber: rempt.Feb 18 2019, 1:58 PM

I get two warnings:

QObject::connect: No such signal KisCanvasController::canvasOffsetXChanged(qreal) in /home/boud/dev/krita/plugins/dockers/overview/overviewdocker_dock.cpp:106
QObject::connect: (receiver name: 'OverviewDocker')
QObject::connect: No such signal KisCanvasController::canvasOffsetXChanged(qreal) in /home/boud/dev/krita/plugins/dockers/overview/overviewdocker_dock.cpp:106
QObject::connect: (receiver name: 'OverviewDocker'

And there's something weird with the mirror button: it seems to lack the toggled indication the eraser button and so on have.

woltherav updated this revision to Diff 51964.Feb 18 2019, 2:09 PM

Fixed the signal, however, I do not know why the eraser button has a highlight and this button doesn't. When putting the action on the toolbar it also doesn't have a highlight.

The patch looks and work okay here :)

dkazakov accepted this revision as: dkazakov.Feb 18 2019, 2:22 PM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 18 2019, 2:27 PM
This revision was automatically updated to reflect the committed changes.