Adds Tool Specific Toolbar #1586

ketanhwr · 2017-06-01T10:52:15Z

@bjorn, @Ablu : I've added a tool specific toolbar. It's not complete right now.

I've added the tools specific to stamp brush and bucket fill tool only, i.e. Flipping and Rotating, and the Random tool as well. I've attached a screenshot as well. The toolbar is visible after the Offset Layer tool. If the selected tool is changed to something else, the tools are not visible anymore.

I will have to extend this only to add specific tools for other tools, so I thought of making this pull request.

Ablu · 2017-06-02T04:30:02Z

src/tiled/mapeditor.cpp

+        connect(mFlipHorizontalButton, &QToolButton::clicked, this, &MapEditor::flipHorizontally);
+        connect(mFlipVerticalButton, &QToolButton::clicked, this, &MapEditor::flipVertically);
+        connect(mRotateLeft, &QToolButton::clicked, this, &MapEditor::rotateRight);
+        connect(mRotateRight, &QToolButton::clicked, this, &MapEditor::rotateLeft);


hm. you create a lot of objects in the code before... Are those ever deleted? Does mToolSpecificToolBar->clear(); do this?

Hmm..looks like I will need to store all the objects too in a list probably and use qDeleteAll

ketanhwr · 2017-06-02T06:59:09Z

I'm thinking it would be a better option to make a new class ToolSpecificToolBar. @Ablu what do you think? It could be easily extended that way.

Ablu · 2017-06-02T09:01:35Z

I think one option could be to simply create one toolbar for each editing context and then simply toggle the visibility according to the current context. That would be easier to handle regarding deletion and recreation of buttons. Also if later other editing contexts might require something different than a toolbar, or require special toolbars that should be easier since there is no dependency on a toolbar then. But maybe @bjorn should comment on this...

ketanhwr · 2017-06-02T10:17:38Z

@Ablu you can have a look.

I also tried the other way round: Basically, I instantiated a QToolButton for each of the tool (Random, Flipping, Rotating) in the constructor of the ToolSpecificToolBar and I added or removed the same instances when the selected tool was changed. But it didn't seem to work. The QToolButton would be visible only for the first time. I guess QToolbar::clear() actually clears the memory too.

So the current implementation is instantiating a new QToolButton every time the selected tool is changed.

Ablu · 2017-06-03T19:04:47Z

I guess @bjorn has to do this review... I do not have too much experience with QtGui... I still think that having multiple toolbars and only toggling the visibility might be another clean approach... But I do not want to rate them against each other :)

bjorn

I think in general this is a good start, though eventually populating the tool bar should be left to the tools. A new virtual function would be added in AbstractTool like virtual void populateToolBar(QToolBar* toolBar);, which is called when the tool is selected. Then we'd need to find a good way of sharing common functionality (actions working on the active tile stamp) between the stamp brush and fill tool.

bjorn · 2017-06-03T20:03:38Z

src/tiled/mapeditor.cpp

@@ -669,6 +609,12 @@ void MapEditor::rotate(RotateDirection direction)
    }
 }

+void MapEditor::random(bool value)


Please call this setRandom.

bjorn · 2017-06-03T20:05:25Z

src/tiled/stampbrush.h

@@ -64,6 +64,8 @@ class StampBrush : public AbstractTileTool
     */
    const TileStamp &stamp() const { return mStamp; }

+    bool random() { return mIsRandom; }


I would call the getter isRandom as well. Also it should be marked const.

bjorn · 2017-06-03T20:08:49Z

src/tiled/toolspecifictoolbar.cpp

+
+void ToolSpecificToolBar::addFlippingTools()
+{
+    QToolButton *mFlipHorizontalButton = new QToolButton(this);


Try to rely on QAction when possible (and I think it's possible for all actions you're currently adding). That way, you can instantiate the action once and just add it to the tool bar when applicable. It won't get deleted upon calling clear, which is good.

Oh that's exactly what I was looking for!

bjorn · 2017-06-03T20:11:18Z

src/tiled/toolspecifictoolbar.cpp

+
+void ToolSpecificToolBar::retranslateUi()
+{
+    // TODO


If you rely on QAction, you can set their tool tips here.

bjorn · 2017-06-03T20:16:41Z

src/tiled/toolspecifictoolbar.h

+class ToolSpecificToolBar : public QToolBar
+{
+public:
+    ToolSpecificToolBar(QWidget *parent = nullptr, MapEditor *mapEditor = nullptr);


Please make the parent the last parameter, which is a general convention followed in Qt.

Also, probably the MapEditor parameter should not default to nullptr?

Then what should be the default parameter for it?

It would simply have no default.

It gave off an error when I did that 🤔

It would do that if you forget the first part of my initial comment, which was to make the parent parameter the last one. In C++, once some parameter is given a default value, all parameters after that also need a default value.

Oh yes, shit 🤦‍♂️

ketanhwr · 2017-06-05T09:26:12Z

Alright so how about this:

Add a function virtual void populateToolBar(ToolSpecificToolBar* toolBar) in AbstractTool
The ToolSpecificToolBar would hold the QAction instances of all the tools like Flipping/Rotating/etc.
Whenever a tool is activated, the populateToolBar would call the functions like addFlippingTools, etc to add the QAction instances to the toolbar.

bjorn · 2017-06-05T10:02:53Z

That would be a good intermediate step, though eventually I think the parameter should be simply a QToolBar pointer and the QAction instances should be owned by the tools.

ketanhwr · 2017-06-05T10:09:11Z

So there's no need for an intermediate class ToolSpecificToolBar then, I guess. It can be created in MapEditor only like the other toolbars.

bjorn · 2017-06-05T10:27:44Z

Right, with the important difference, that the content of the tool bar is determined by the currently selected tool.

ketanhwr · 2017-06-06T06:01:48Z

Currently, all the functions handling rotation and flipping are in MapEditor class. If these are shifted to StampBrush and BucketFillTool then we won't be able to share common tile stamps. How do I handle this? Should I pass a reference of MapEditor instance to populateToolBar too? In this way, we can connect the QAction to the functions in populateToolBar which solves the problem

bjorn · 2017-06-12T09:55:16Z

Currently, all the functions handling rotation and flipping are in MapEditor class. If these are shifted to StampBrush and BucketFillTool then we won't be able to share common tile stamps. How do I handle this? Should I pass a reference of MapEditor instance to populateToolBar too? In this way, we can connect the QAction to the functions in populateToolBar which solves the problem

You do not not need to use MapEditor directly to share the same stamp between these tools. Instead, you could rely on a connection like this one:

connect(mStampBrush, &StampBrush::stampCaptured, this, &MapEditor::setStamp);

So when the StampBrush flips the stamp, it could also emit stampCaptured (which could probably be renamed to stampChanged), and a similar signal can be added to BucketFillTool.

ketanhwr · 2017-06-19T05:41:21Z

@bjorn, I've added a new class StampActions. I first started with AbstractStampTool class but there was nothing common except the newly added QActions so I reverted and made the class StampActions.

bjorn

Hmm, at least mStamp and mRandom would be still in common, and when moved to the StampActions class would avoid more duplicated code. Let me know how you feel about that.

bjorn · 2017-06-19T10:00:56Z

src/tiled/stampactions.cpp

+    QIcon mFlipHorizontalIcon(QLatin1String(":images/24x24/flip-horizontal.png"));
+    QIcon mFlipVerticalIcon(QLatin1String(":images/24x24/flip-vertical.png"));
+    QIcon mRotateLeftIcon(QLatin1String(":images/24x24/rotate-left.png"));
+    QIcon mRotateRightIcon(QLatin1String(":images/24x24/rotate-right.png"));


Naming style: don't name local variables with the member prefix.

bjorn · 2017-06-19T10:06:24Z

src/tiled/stampactions.h

+    QAction *flipHorizontal() { return mFlipHorizontal; }
+    QAction *flipVertical() { return mFlipVertical; }
+    QAction *rotateLeft() { return mRotateLeft; }
+    QAction *rotateRight() { return mRotateRight; }


All these getters should be const. However, when mStamp and mRandom are moved into this class, you only need getters (and signals) for those and not for all the actions.

bjorn · 2017-06-19T11:26:48Z

src/tiled/bucketfilltool.h

@@ -64,9 +65,18 @@ class BucketFillTool : public AbstractTileTool
     */
    const TileStamp &stamp() const { return mStamp; }

+    bool isRandom() { return mIsRandom; }


Please remove (since it's not used) or mark const if kept.

bjorn · 2017-06-19T11:27:04Z

src/tiled/stampbrush.h

@@ -64,6 +65,10 @@ class StampBrush : public AbstractTileTool
     */
    const TileStamp &stamp() const { return mStamp; }

+    bool isRandom() { return mIsRandom; }


Please remove (since it's not used) or mark const if kept.

bjorn · 2017-06-19T11:41:23Z

Thanks! I'm looking forward to see the use of this toolbar extended to more tools. :-)

* Fixed crash when editing collision when tile image wasn't loaded When opening a tileset it can happen that the tileset image fails to load. In this case, opening the tile collision editor could lead to a crash. * GmxPlugin: Fixed tile type inheritance for tile objects Now tile objects of which the tile has a type defined are exported as instances of this type of object in the GameMaker room file. * Added toolbar for stamp brush and bucket fill tool (mapeditor#1586) This adds a new tool-specific toolbar that can be used by tools. Currently contains actions for rotating/flipping stamp and toggling random mode. Closes mapeditor#1084 * docs: Fixed link to other page * QtPropertyBrowser: Removed deprecation warnings The classes were deprecated in Qt 5.0 and warnings have been added in Qt 5.7. * Fixed rendering of tile object outlines for resized objects They were taking the size of the image instead of the size of the object, which means this was broken since Tiled 0.12. * Fixed rendering of tile objects when the image couldn't be loaded If the tile was found but its image failed to load, tile objects would not render at all and due to a broken boundingRect be also impossible to interact with. Now they render as the special "missing image marker" and can be interacted with. * More fixes for labels of objects nested in a group layer * Fixed labels shown on objects hidden via a group layer * Fixed updating of label visibility when toggling group layer visibility * Fixed updating of label positions when moving a group layer When moving a group layer, any labels present for objects nested within that group layer need to be synchronized. * GmxPlugin: Added support for defining views with objects (mapeditor#1621) * Fixed hang when trying to fill with a pasted stamp Since 688ec7d the size of a copied map is set to 0x0 instead of matching the tile layer's size. It was supposed to be irrelevant, but as it turns out TileStamp::maxSize was based on the size of the map instead of the size of the tile layer. This could lead to an infinite loop in fillWithStamp in bucketfilltool.cpp. Closes mapeditor#1617 Closes mapeditor#1624 * Restored Ctrl+N shortcut on "New Map" action There isn't really a good reason not to have this shortcut. Eventually it may pop up a dialog where you can pick what you want to create, but since it's more common to create new maps than new tilesets we can just do that for now. * Use initializer list for quick-stamp keys * Introduced TilesetDocumentsModel and its sort-filter model companion This model lists the tileset documents that are currently open, and the sort-filter version sorts them by name and filters out tilesets that are embedded in other maps than the current one. This model extracts part of the logic from TilesetDock, so that it could be reused by an updated TerrainModel. The TerrainModel currently only lists terrains from tilesets that are already part of the map, but it should display all loaded external tilesets. * libtiled-java: Fixed wrong exception being caught in TileSet (mapeditor#1629) * Display all tilesets with terrain in the Terrains view Except for tilesets that are embedded into another map than the current one, the Terrains view now displays all tilesets that have terrains defined. The Terrain Brush will now automatically add the tileset of the currently selected terrain to the map when it isn't already present. * Show custom properties on tiles and terrains in the map editor While still not editable, this change shows these properties in a read-only fashion. It is often useful to see them, as indicated by multiple users on the forum. * Bumped version to 1.0.2 and updated NEWS file * Adds option to lock/unlock layer (mapeditor#1627) Locking a layer prevents modifications to the layer by the tools, as well as by some actions like cut and delete. Modifications to objects are prevented by making them not selectable. Closes mapeditor#734 * Fixed tool tips on flipping and rotating stamp actions

Adds specific toolbar and tools for stamp brush and bucket fill tool

927f44c

Ablu reviewed Jun 2, 2017

View reviewed changes

ketanhwr added 3 commits June 2, 2017 15:14

Makes a new class ToolSpecificToolBar

5a0a849

Fixes indentation

8b9df68

small indentation

61f8f88

bjorn requested changes Jun 3, 2017

View reviewed changes

Reverts new class

8d09161

bjorn changed the title ~~Adds Tool Specific Toolbar - #1084~~ Adds Tool Specific Toolbar Jun 8, 2017

ketanhwr added 3 commits June 15, 2017 13:28

Tools populate toolbar themselves

40c6446

Change shortcut and tooltip when language is changed

e016bbb

Adds new class StampActions

1d347f4

Removes unnecessary forward declarations

d2821ff

bjorn requested changes Jun 19, 2017

View reviewed changes

Minor changes

91faed4

bjorn requested changes Jun 19, 2017

View reviewed changes

Removes unnecessary getters

743fa9d

bjorn merged commit 43b9da9 into mapeditor:master Jun 19, 2017

boaromayo mentioned this pull request Aug 3, 2017

Update forked repo (#1) #1675

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Tool Specific Toolbar #1586

Adds Tool Specific Toolbar #1586

ketanhwr commented Jun 1, 2017 •

edited by bjorn

Loading

Ablu Jun 2, 2017

ketanhwr Jun 2, 2017

ketanhwr commented Jun 2, 2017 •

edited

Loading

Ablu commented Jun 2, 2017

ketanhwr commented Jun 2, 2017 •

edited

Loading

Ablu commented Jun 3, 2017 •

edited

Loading

bjorn left a comment

bjorn Jun 3, 2017

bjorn Jun 3, 2017

bjorn Jun 3, 2017

ketanhwr Jun 4, 2017

bjorn Jun 3, 2017

bjorn Jun 3, 2017

ketanhwr Jun 4, 2017

bjorn Jun 4, 2017

ketanhwr Jun 4, 2017

bjorn Jun 4, 2017

ketanhwr Jun 4, 2017

ketanhwr commented Jun 5, 2017

bjorn commented Jun 5, 2017

ketanhwr commented Jun 5, 2017

bjorn commented Jun 5, 2017

ketanhwr commented Jun 6, 2017

bjorn commented Jun 12, 2017

ketanhwr commented Jun 19, 2017

bjorn left a comment

bjorn Jun 19, 2017

bjorn Jun 19, 2017

bjorn Jun 19, 2017

bjorn Jun 19, 2017

bjorn commented Jun 19, 2017

Adds Tool Specific Toolbar #1586

Adds Tool Specific Toolbar #1586

Conversation

ketanhwr commented Jun 1, 2017 • edited by bjorn Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketanhwr commented Jun 2, 2017 • edited Loading

Ablu commented Jun 2, 2017

ketanhwr commented Jun 2, 2017 • edited Loading

Ablu commented Jun 3, 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

Choose a reason for hiding this comment

ketanhwr commented Jun 5, 2017

bjorn commented Jun 5, 2017

ketanhwr commented Jun 5, 2017

bjorn commented Jun 5, 2017

ketanhwr commented Jun 6, 2017

bjorn commented Jun 12, 2017

ketanhwr commented Jun 19, 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

Choose a reason for hiding this comment

bjorn commented Jun 19, 2017

ketanhwr commented Jun 1, 2017 •

edited by bjorn

Loading

ketanhwr commented Jun 2, 2017 •

edited

Loading

ketanhwr commented Jun 2, 2017 •

edited

Loading

Ablu commented Jun 3, 2017 •

edited

Loading