Rewrote and simplified the old Pan tool based on KisTool. Added some helper functions to KoCanvasController to make the code a bit cleaner.
This solves bug 365933 (Pan tool only).
Details
Details
- Reviewers
rempt woltherav dkazakov - Group Reviewers
Krita - Commits
- R37:aaaf1eea993d: Rewrote the Pan tool based on KisTool. Fixes pan issues when mirroring is…
- Select the pan tool
- Click and drag on the canvas
- Use arrow keys
Diff Detail
Diff Detail
- Repository
- R37 Krita
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| plugins/tools/basictools/kis_tool_pan.h | ||
|---|---|---|
| 33 | Accidental leftover from testing some functions from KisTool. I'll remove Q_SLOTS. | |
| libs/flake/KoCanvasController.h | ||
|---|---|---|
| 260 | The old implementation of the pan tool had an implicit dependency on KoCanvasControllerWidget due to having to access the scrollbars. This removes that dependency. There are other ways of providing this, but given there's only one use-case currently this felt cleanest. Another solution would be something like panDiscrete(int x, int y), though that can easily be added later if there is ever a need. | |
| plugins/tools/basictools/kis_tool_pan.cpp | ||
| 47 | This avoids the pan bug the old implementation suffered from when mirroring is enabled. The old implementation used document coordinates for some reason, and then converted those to view coordinates. The problem with that is that panning is done on a widget level (despite what the docs say), so the view coordinates would be inverted from what the pan function expected. Since we're only interested in delta cursor movement, we might as well use widget coordinates. | |
| 61 | This is in keeping with the old behaviour, where panning via keys would track the scrollbars. | |
Comment Actions
The location of the new pan tool is wrong; apart from that, all is fine.
| plugins/tools/basictools/kis_tool_pan.cpp | ||
|---|---|---|
| 89 | Hm... This probably isn't correct: now the pan tool is placed in the middle of the toolbar, instead of at the bottom, like it used to be. | |
Comment Actions
Hi, @victorw!
Did you do connect the arrow keys in reversed way intentionally? I would expect the right arrow key to move the canvas to the right and left arrow move it to the left. At least that is how most of the applications work... And in your patch it works in a reversed way. The arrows move the scrollbars instead.
Comment Actions
Yes, it's intentional, though only because that's the current behaviour (both in master and 3.1.4). I thought it was a bit weird too, but I guess whoever originally wrote it wanted it to behave like the arrow keys in generic applications, i.e. they move the scrollbars... Then again, the original implementation wasn't Krita specific.
I'm in favour of changing it so it moves the canvas, but I have no strong opinion on the matter. Since this feels like a relatively minor thing, I'll change it if no one objects. :)
Edit: I updated the code to pan the canvas when you use the arrow keys. Arguably it makes more sense, but it's different from the old behaviour. If there are no objections I'll submit this version.
| plugins/tools/basictools/kis_tool_pan.cpp | ||
|---|---|---|
| 89 | I can change the section. I only changed it because the comment for TOOL_TYPE_VIEW specifically says it's for "Tools that affect the canvas: pan, zoom, etc." This section is also used by the measurement tool, which is in the same category. I don't mind either way though IMO it makes sense to group these type of tools. Should I change it back? | |
Comment Actions
- Inverted pan movement when using arrow keys. This is a change from the original behaviour, but could be considered more correct in the given context.
- Switched section to the old section so the tool icon doesn't move in the toolbox.
| plugins/tools/basictools/kis_tool_pan.cpp | ||
|---|---|---|
| 89 | I updated the code to use the old section (navigationToolType()). There seem to be some tech debt here as there are two definitions for sections. One is defined in KoToolFactoryBase.h and one is defined in kis_tool.h. They should probably be unified, which would require taking inventory of all tools and creating new groups suitable for them. Maybe a future cleanup task? | |
Comment Actions
Hi, @victorw!
Now the patch looks perfectly correct! :)
You mean the location in the toolbox, or the file?
I think Boud meant the location of the button on the toolbox. The file is looks to be placed correctly.
| plugins/tools/basictools/kis_tool_pan.cpp | ||
|---|---|---|
| 89 | Yes, that is an artifact of forking from old Calligra code. It might be a future task, yes. But since it still works and satisfies everyone, I wouldn't spend time on it. Next time when painters ask for the tools reordering, we can refactor it as well :) | |