Add optional undo, redo, crop, scale and modify canvas buttons to dock widgets

[precla]

Add undo/redo/crop/scale/modify canvas buttons when required. Since those are not always needed.
For example ksnip has those built-in.

[precla]

Ok, now this one lacks functionality when clicking on the buttons and the buttons are always visible.
Quite a lot of classes "above" this. So I'm not quite sure how to call, for example CoreView::undoAction, from within TransformationTools::undoAction. Can you assist with this?

[precla]

Screenshot:

[kevincolyer]

Thanks for working on this. Will be a great addition!

[precla]

Done, tested & working.
Test & code review please :)

[DamirPorobic]

I'll have look into it today, yesterday evening GitHub was done for me.

One thing that I've already noticed, not wrong just might be misleading. The name "Transformation" might be used in item operations too, there would it be like rotate, scale and so on, that would also be Transformation. But to be honest, I've got no better name right now for it.

[DamirPorobic]

I'm missing an option to disable the Transformation Settings Widget explicitly. Or am I missing something?

[precla]

I'm missing an option to disable the Transformation Settings Widget explicitly. Or am I missing something?

You can enable it with:
kImageAnnotator->showTransformationSettingsWidget()

[DamirPorobic]

You can enable it with:
kImageAnnotator->showTransformationSettingsWidget()

Strange, I see the method in the code but not in the Review on GitHub. Looks like one commit is missing in the review, I also don't see the change in example but do see it in the code.

[DamirPorobic]

One problem though, currently we setup the widgets and call the restoreDockWidgets to load the last saved position of widgets from config. Now the Transformation widget is added after the restoreDockWidgets call so it always ends up in the bottom right corner. Basically you cannot persist the position of the widget. Might be enough to just call the restoreDockWidgets method in the showTransformationSettingsWidget method after adding the widget.

[precla]

One problem though, currently we setup the widgets and call the restoreDockWidgets to load the last saved position of widgets from config. Now the Transformation widget is added after the restoreDockWidgets call so it always ends up in the bottom right corner. Basically you cannot persist the position of the widget. Might be enough to just call the restoreDockWidgets method in the showTransformationSettingsWidget method after adding the widget.

Added, seems to do the trick.

All fine now? Except for the naming. Transformation doesn't really fit, since this includes undo/redo - which are not that much of a transformation. I also couldn't come up with something better.. :(
Maybe Modification? You got any ideas?

[DamirPorobic]

Modification might be a bit better but then again, what are you modifying, could be anything. Problem is that the operations are mixed, undo/redo is an overall operation affecting items and image, scale and crop only affect the image and modify canvas effects only what's outside the image, there is nothing common except that they're available through other calls. No clue. Should we leave it as it is?

[precla]

Yeah, undo/redo is not a modification per se. Can't come up with something else atm.. Leave it as is and 'one day' when we come up with something different, change it?

Shall I squash commits?

[DamirPorobic]

Yeah, let's go with that. The only drawback is that the showTransformationSettingsWidget is part of our external interface so we cannot just like that rename it but yeah, the "one day" might be far away :)

You can squash it, I'll have a quick look through and test later today when I find time and merge it then.

[precla]

Yeah, let's go with that. The only drawback is that the showTransformationSettingsWidget is part of our external interface so we cannot just like that rename it but yeah, the "one day" might be far away :)

You can squash it, I'll have a quick look through and test later today when I find time and merge it then.

Hm, maybe General Selection , since General Settings is already taken (and only has Zoom in it) ? Then we'd have showGeneralSelectionWidget. When I think about it, undo/redo/crop/... ain't settings, so renaming to selection should make sense?

a) showGeneralSelectionWidget (I'd prefer this one), and rename all of TransformationSettings into GeneralSelection

b) showTransformationSelectionWidget

[DamirPorobic]

Yeah, something General doesn't sound bad, it's not settings indeed but doesn't sound like selection either. Maybe Controls? GeneralControlsWidget or just ControlsWidget?

[precla]

ControlsWidget sounds great and is short. Will change now to that.

[DamirPorobic]

Looks good to me. The unused variable should be remove and the names are up to you, if you want to rename the signals.

[DamirPorobic]

Thanks for providing this PR, merged.

[DamirPorobic]

Run into an issue now that I was implementing support for this in ksnip. This is a setting basically but you can only show it and never hide. Also from the naming, we have showCrop(), showRotator() (btw this one is missing in the controls, it allows rotating the image) which basically show a temporary window, but this here should be more like an option or setting, do we want to show it or not. We should maybe rename it to something like isControlsWidgetVisible(bool isVisible) or setControlsWidgetEnabled(bool isEnabled), dunno, something in that direction. Do you have time to make another PR?

[precla]

Oh, sorry for that one. Haven't tested it in ksnip.
You mean rename and change the showControlsWidget so it allows to either have it enabled or disabled? Yeah, currently it only allows to never show or show and never hide.. So, make the following changes in a new PR?

• showControlsWidget() -> setControlsWidgetEnabled(bool isEnabled)
• add showRotator() into Controls

[DamirPorobic]

Not an issue, you were not supposed to test it in ksnip, I just thought it would be nice to make it there available too if someone want to use it. There it's just a checkbox which is unchecked by default. Not I can enable the control but not disable, I have to restart ksnip xD

Yes, exactly, those two changes. Now looking at those two options, isControlsWidgetVisible(bool isVisible) seems to be more accurate because it's visible or not, versus enabled which would be kind of grayed out or not.

Thanks again!

[precla]

Not I can enable the control but not disable, I have to restart ksnip xD

The mother of all solutions in IT :D

Yes, exactly, those two changes. Now looking at those two options, isControlsWidgetVisible(bool isVisible) seems to be more accurate because it's visible or not, versus enabled which would be kind of grayed out or not.

I'm on my way to add those :)

[DamirPorobic]

yeah :D

[precla]

:D
btw. started to work on this and just to be sure:
a) isControlsWidgetVisible(bool isVisible) has to make the Controls appear and disappear? in that case I'd go for setControlsWidgetVisible(bool setVisible) because we want to set/unset something
or
b) only give feedback if Controls is visible: return true if currently visible, if not return false?

if a) -> we only have insertDockWidget() , nothing to hide it. Except QWidget's hide(), to hide the AnnotationDockWidget created for Controls.

[precla]

info: Rotate button is in a separate MR: #264

[DamirPorobic]

Definitely a) and yes, the naming you have there is correct.

QMainWindow should have a void removeDockWidget(QDockWidget *dockwidget); for removing dock widgets, you probably just need to additionally remove it from the mDockWidgets List.

[Pointedstick]

Nice!