Added Move selection Feature #1607

ketanhwr · 2017-06-10T10:37:58Z

This addresses #650.

It works fine as of now, but I need feedback regarding the flow of this tool. You need to select an area with the tile selection tool, and then select this tool and then move the selection to someplace else. I have a few doubts in mind regarding the flow but I think a feedback would be great!

Ablu · 2017-06-10T15:31:24Z

A few points I realized while testing:

When starting to drag, but still staying on the same tile (so only drag like a pixel to the left or right), the whole map is overlayed with the light blue/gray
Sometimes I get a situation like this:

The lower row is nothing what I selected... seems to be a small graphical glitch... It seems to preview one tile to much in width and height. Also the display of those lines seems to lag behind while dragging...

Generally I think the flow of the tool is fine... The only thing which could be annoying is that there is no thing like a floating layer like in Gimp... So if you move tiles on crowded layers you might accidently delete / override tiles...

Ablu

Did a very undetailed review. I guess @bjorn needs to check whether the approach is correct.

Ablu · 2017-06-10T15:34:46Z

src/tiled/moveselectiontool.cpp

+void MoveSelectionTool::tilePositionChanged(const QPoint &pos)
+{
+    if (mDragging) {
+


unneeded empty line

Ablu · 2017-06-10T15:39:43Z

src/tiled/moveselectiontool.cpp

+    TileLayer *tileLayer = dynamic_cast<TileLayer*>(currentLayer);
+    const QRegion &selectedArea = mapDocument()->selectedArea();
+
+    ClipboardManager::instance()->copySelection(mapDocument());


I still think that there should be no ClipboardManager involved here... One would not expect to loose the current clipboard content when using the tool. It should be possible to store the moved selection in the tool.

Can't you simply use the mPreviewLayer variable for this?

Oh yeah. I didn't realise that, I'll look into this!

Ablu · 2017-06-10T15:41:19Z

src/tiled/moveselectiontool.cpp

+    if (map)
+        tilesetManager->removeReferences(map->tilesets());
+
+    brushItem()->clear();


I think the preview layer should be cleared here too.

ketanhwr · 2017-06-12T10:42:43Z

@Ablu: The clipboard is no longer needed now. I've used the preview layer to paint only. There are still a few bugs.

So if you move tiles on crowded layers you might accidently delete / override tiles...

This can be solved by: Whenever the tool is selected, the selected area remains same, as in, even if it overrides the tiles when you move the selection, it won't actually delete them unless you choose some other tool. I will have to save the preview layer when the tool is first activated and paint when the tool is deactivated.

ketanhwr · 2017-06-12T12:33:30Z

@Ablu, I've probably fixed all the issues that you've mentioned.

Sometimes I get a situation like this:

I'm not able to reproduce it, it might as well be a graphical error only.

bjorn

Yep, this tool is definitely full of challenges. I've tried it out and inspected the code, and here's my feedback. :-)

bjorn · 2017-06-13T08:21:33Z

src/tiled/moveselectiontool.cpp

+void MoveSelectionTool::activate(MapScene *scene)
+{
+    AbstractTileTool::activate(scene);
+    cut();


I think you should wait with the cutting until the user starts to drag the selection.

bjorn · 2017-06-13T08:25:41Z

src/tiled/moveselectiontool.cpp

+        }
+
+        mLastUpdate = pos;
+    }


I think you should refresh the cursor also in this function, and change it to a hand when the user will start a drag operation (if the position is in the selected area). This makes it easier to anticipate the action done when clicking. See also the change of cursor when hovering an object.

bjorn · 2017-06-13T08:28:24Z

src/tiled/moveselectiontool.cpp

+        QRegion selectedArea = mapDocument()->selectedArea();
+        selectedArea.translate(offset);
+
+        mapDocument()->undoStack()->push(new ChangeSelectedArea(mapDocument(), selectedArea));


This is not a good idea since it is part of the undo history. I think the selection should be cleared when you start to drag (as part of the cut, essentially), at which point the brush item will take over the highlighting of the floating tiles. Once the move operation is done, the selection can stay cleared.

bjorn · 2017-06-13T08:29:07Z

src/tiled/moveselectiontool.cpp

+
+        if (mPreviewLayer) {
+            mPreviewLayer->setX(mPreviewLayer->x()+offset.x());
+            mPreviewLayer->setY(mPreviewLayer->y()+offset.y());


Coding style: please put spaces around the + operator.

bjorn · 2017-06-13T08:31:23Z

src/tiled/moveselectiontool.cpp

+
+void MoveSelectionTool::mouseReleased(QGraphicsSceneMouseEvent *)
+{
+    if (mDragging) {


You'll probably want to ignore all but the left mouse button here.

bjorn · 2017-06-13T09:00:01Z

src/tiled/moveselectiontool.h

+
+    void activate(MapScene *scene) override;
+    void deactivate(MapScene *scene) override;
+


You'll want to override the default implementations of mouseEntered and mouseLeft, since they're showing/hiding the preview item, which is not what you want for this tool.

bjorn · 2017-06-13T09:07:31Z

src/tiled/moveselectiontool.cpp

+    TileLayer *tileLayer = dynamic_cast<TileLayer*>(currentLayer);
+    const QRegion &selectedArea = mapDocument()->selectedArea();
+
+    TileLayer *brushLayer = tileLayer->copy(tileLayer->bounds());


You should not copy the entirely layer (and Layer::bounds is technically not in the right coordinate space to use as a parameter to Layer::copy, though it won't run into issues with layers at 0,0 which is all layers at the moment). Essentially, I think you want:

mPreviewLayer = SharedTileLayer(tileLayer->copy(selectedArea.translated(-tileLayer->position())));

And since this only copied the relevant region, you'll need to set the initial position:

mPreviewLayer->setPosition(selectedArea.boundingRect().topLeft());

bjorn · 2017-06-13T09:09:13Z

src/tiled/moveselectiontool.cpp

+    mPreviewLayer = SharedTileLayer(brushLayer);
+
+    brushItem()->setTileLayer(mPreviewLayer);
+    brushItem()->setTileRegion(mapDocument()->selectedArea());


Please use the version of setTileLayer that also takes a region, otherwise setTileLayer will first compute the region from the tile layer, which is a waste of time:

brushItem()->setTileLayer(mPreviewLayer, selectedArea);

bjorn · 2017-06-13T09:16:13Z

src/tiled/moveselectiontool.cpp

+    brushItem()->setTileRegion(mapDocument()->selectedArea());
+
+    QUndoStack *stack = mapDocument()->undoStack();
+    stack->beginMacro(tr("Move Selection"));


I think it's a bad idea to leave this macro open, since it will be hard to ensure that it is eventually ended, and even harder to make sure that no other changes are pushed to the stack in between. Maybe it's better just to end this macro here and deal with the fact that the user may undo after starting a move operation.

bjorn · 2017-06-13T09:20:02Z

src/tiled/moveselectiontool.cpp

+            mPreviewLayer->setX(mPreviewLayer->x()+offset.x());
+            mPreviewLayer->setY(mPreviewLayer->y()+offset.y());
+            brushItem()->setTileLayer(mPreviewLayer);
+            brushItem()->setTileRegion(mapDocument()->selectedArea());


Please use the version of setTileLayer that takes the region as second parameter. Also, since we shouldn't rely on the selected area, you could instead translate the current tile region by the offset as well: brushItem()->tileRegion().translated(offset).

ketanhwr · 2017-06-13T11:01:25Z

@bjorn I've made the changes, have a look now 🙂

bjorn

I've provided some minor inline comments, but here's the big suggestion:

You need to select an area with the tile selection tool, and then select this tool and then move the selection to someplace else. I have a few doubts in mind regarding the flow but I think a feedback would be great!

What do you think about trying to merge the new Move tool with the selection tools? Since we have rectangular select, magic wand and select-same-tile, I imagine we could introduce AbstractSelectionTool, which would be used to add the ability to drag the selected tiles to all selection tools. In GIMP, you hold Ctrl+Alt modifiers while dragging to trigger this move behavior, and I think we could do the same in Tiled.

The current behavior is quite good (except, I think clicking outside of the preview should still perform the paste operation), but I also have my doubts about this switching between tools.

bjorn · 2017-06-15T07:35:33Z

src/tiled/moveselectiontool.cpp

+        const int dragDistance = (mMouseScreenStart - screenPos).manhattanLength();
+
+        if (dragDistance >= QApplication::startDragDistance() / 2) {
+            if (!mCut) {


I guess you don't actually need mCut, since you could use if (!mPreviewLayer) instead.

bjorn · 2017-06-15T07:36:32Z

src/tiled/moveselectiontool.cpp

+    if (event->button() != Qt::LeftButton)
+        return;
+
+    if (mDragging) {


No need for this condition.

bjorn · 2017-06-15T07:38:58Z

src/tiled/moveselectiontool.cpp

+    QUndoStack *stack = mapDocument()->undoStack();
+    stack->beginMacro(tr("Move Selection"));
+
+    if (!selectedArea.isEmpty()) {


Actually the selected area can't be empty when cut is called, right? Maybe it's a good idea to make sure anyway, but then this check should be moved up.

bjorn · 2017-07-10T10:59:05Z

Just to add an explanatory note: this PR was closed because @ketanhwr introduced an AbstractTileSelectionTool in #1620 and will now merge the functionality of the Move Selection tool added here with that class.

ketanhwr added 5 commits June 9, 2017 13:45

basic code

5390462

Major bug fix

bfa5e2d

Selected area moves along with dragging

7600a4c

Adds preview

33a89c4

Adds new line at the end of file

36817d0

Ablu suggested changes Jun 10, 2017

View reviewed changes

Clipboard is no longer needed

5dada3f

ketanhwr added 4 commits June 12, 2017 17:38

Preview doesn't get deleted when moved out of the layer boundary

86b65dc

Fixes undo/redo of move selection

d83600e

Bug fix

5285d70

Tiles do not override other tiles

8a19709

bjorn requested changes Jun 13, 2017

View reviewed changes

ketanhwr added 5 commits June 13, 2017 15:45

Requested changes

a6703a8

Bug fix and ignore mouse release unless LMB

b0759c6

Another bug fix

aae2da9

Cursor changes when moved over selected area

c7ca7f3

Remember target layer

76f8c28

bjorn requested changes Jun 15, 2017

View reviewed changes

ketanhwr added 2 commits June 19, 2017 22:27

Clicking outside the selection pastes it

d5e4ebd

Ctrl modifier is used to duplicate the selection

c36e767

ketanhwr closed this Jul 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Move selection Feature #1607

Added Move selection Feature #1607

ketanhwr commented Jun 10, 2017 •

edited

Loading

Ablu commented Jun 10, 2017

Ablu left a comment

Ablu Jun 10, 2017

Ablu Jun 10, 2017

Ablu Jun 10, 2017

ketanhwr Jun 10, 2017

Ablu Jun 10, 2017

ketanhwr commented Jun 12, 2017

ketanhwr commented Jun 12, 2017 •

edited

Loading

bjorn left a comment

bjorn Jun 13, 2017

bjorn Jun 13, 2017

bjorn Jun 13, 2017

bjorn Jun 13, 2017

bjorn Jun 13, 2017

bjorn Jun 13, 2017

bjorn Jun 13, 2017

bjorn Jun 13, 2017

bjorn Jun 13, 2017

bjorn Jun 13, 2017

ketanhwr commented Jun 13, 2017

bjorn left a comment

bjorn Jun 15, 2017

bjorn Jun 15, 2017

bjorn Jun 15, 2017

bjorn commented Jul 10, 2017


		void activate(MapScene *scene) override;
		void deactivate(MapScene *scene) override;

Added Move selection Feature #1607

Added Move selection Feature #1607

Conversation

ketanhwr commented Jun 10, 2017 • edited Loading

Ablu commented Jun 10, 2017

Ablu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketanhwr commented Jun 12, 2017

ketanhwr commented Jun 12, 2017 • edited Loading

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketanhwr commented Jun 13, 2017

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn commented Jul 10, 2017

ketanhwr commented Jun 10, 2017 •

edited

Loading

ketanhwr commented Jun 12, 2017 •

edited

Loading