Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds move selection feature #1647

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ketanhwr
Copy link
Contributor

#650

The tool works when an area is selected and then Ctrl+Alt is used to move the selection. Ctrl+Alt+Shift is used to duplicate the selection but this modifer works weirdly (You need to hold Ctrl+Alt and after that the shift).

@bjorn
Copy link
Member

bjorn commented Jul 11, 2017

I'm a little bit surprised by that it automatically applies the move when I release the mouse button. Why doesn't it stay in a floating state, like the movement tool used to do?

Also I wonder if it's really needed to have Ctrl+Alt as modifier for this. I realize GIMP uses the same modifier, but they also handle just Alt for moving the selection without affecting the image. Either we could eventually also support Alt like that in Tiled, or we could try using just Alt for doing the drag. Or, we could try whether it would work without modifier, just going by "if you start dragging on the selection, without modifier". What do you think?

@ketanhwr
Copy link
Contributor Author

I like the option of just using Alt. Without using the modifer, might get a bit weird and a lot of tricky situations might come I think.

@@ -41,6 +43,7 @@ class AbstractTileSelectionTool : public AbstractTileTool
const QKeySequence &shortcut,
QObject *parent = nullptr);

void mouseMoved(const QPointF &pos,Qt::KeyboardModifiers modifiers) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space missing after , here

@Ablu
Copy link
Contributor

Ablu commented Aug 18, 2017

What is the status on this by the way?

* Holding Alt is enough to do a drag. No need to click inside the
  selection.

* Releasing either Alt or the mouse does not anchor the floating
  selection, so it can be dragged repeatedly.

* The selection is anchored when the tool gets deactivated, you click
  outside of the floating selection or press Enter.
@bjorn
Copy link
Member

bjorn commented Aug 21, 2017

I've pushed a commit with several changes. Unfortunately, I still don't think we can merge this to master because of some remaining issues:

  • It is hardly clear to the user that his selection is floating. Probably the BrushItem should be enhanced with an option to render an animated border around its selected area.

  • Like this, it does not work nicely with the undo system. When you start a drag and then undo, the expected behavior is that the drag is aborted with the tile layer restored. Currently, the layer is restored but the floating selection remains.

  • When you finish a move by anchoring the selection, then undo will only undo the final Paint operation, and doesn't restore the floating selection.

Maybe I'm making it too complicated by allowing repeated drags, but I think it needs to be supported because users will expect it to work and they will need it to work when they need to take the selection somewhere outside of their screen (or revert back to using copy/paste of course).

I think we should look again at other editors to see how they behave regarding such a feature.

@ketanhwr
Copy link
Contributor Author

Maybe the move selection can have a logic similar to object selection tool. It can have the rotating ants animation as well, which would make the feature quite easy to use and understand.

@bjorn bjorn force-pushed the master branch 3 times, most recently from 384d5ac to 2e9a0fb Compare June 25, 2022 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants