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

Add 'New Tileset...' button when no tileset is opened #1789

Merged
merged 8 commits into from Nov 7, 2017

Conversation

Projects
None yet
3 participants
@RhenaudTheLukark
Copy link
Contributor

commented Oct 18, 2017

Feature announced in the issue #1766.

RhenaudTheLukark added some commits Oct 18, 2017

@ketanhwr
Copy link
Contributor

left a comment

Some general suggestions regarding the code style 🙂

@@ -553,6 +576,11 @@ void TilesetDock::deleteTilesetView(int index)
delete view; // view needs to go before the tab
mTabBar->removeTab(index);

// Make the "New Tileset..." special tab reappear if there is no tileset open
if (mTilesets.count() == 0) {

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Oct 18, 2017

Contributor

Can remove the braces since it's only 1 line block.

This comment has been minimized.

Copy link
@RhenaudTheLukark

RhenaudTheLukark Oct 18, 2017

Author Contributor

I've already changed that in the second commit.
Or maybe I didn't?
I'll check again.
EDIT: You were right, I removed the brackets in the first commit but somehow added them again?
Anyway it's fixed now!

@@ -165,7 +165,7 @@ private slots:
TilesetDocumentsFilterModel *mTilesetDocumentsFilterModel;

QTabBar *mTabBar;
QStackedWidget *mViewStack;
QStackedWidget *mSuperViewStack, *mViewStack;

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Oct 18, 2017

Contributor

Please define each variable in a new line.

This comment has been minimized.

Copy link
@RhenaudTheLukark

RhenaudTheLukark Oct 18, 2017

Author Contributor

Will do on the third commit.

RhenaudTheLukark RhenaudTheLukark
@bjorn
Copy link
Owner

left a comment

Thanks for working on this!

I didn't get around to trying it out yet, but here's my comments on the code.

class NewTilesetView : public QWidget
{
public:
explicit NewTilesetView(QWidget *tmb = nullptr)

This comment has been minimized.

Copy link
@bjorn

bjorn Oct 23, 2017

Owner

What's a tmb? Normally this parameter is called the parent.

{
QWidget *w = new QWidget(this);

QPushButton *pbNewTileset = new QPushButton(w);

This comment has been minimized.

Copy link
@bjorn

bjorn Oct 23, 2017

Owner

Please just call it newTilesetButton.

QPushButton *pbNewTileset = new QPushButton(w);
pbNewTileset->setText(QStringLiteral("New Tileset..."));

connect(pbNewTileset, SIGNAL(released()), tmb, SLOT(newTileset()));

This comment has been minimized.

Copy link
@bjorn

bjorn Oct 23, 2017

Owner

At this point, "tmb" is just a widget. It does not indicate what this "newTileset" slot may be, which is bad for code readability.

This comment has been minimized.

Copy link
@RhenaudTheLukark

RhenaudTheLukark Oct 24, 2017

Author Contributor

I changed it, instead of giving a QWidget, I'll specify you need to give the TilesetDock itself, which is what this QWidget is.

@@ -553,6 +576,10 @@ void TilesetDock::deleteTilesetView(int index)
delete view; // view needs to go before the tab
mTabBar->removeTab(index);

// Make the "New Tileset..." special tab reappear if there is no tileset open
if (mTilesets.count() == 0)

This comment has been minimized.

Copy link
@bjorn

bjorn Oct 23, 2017

Owner

Please use mTilesets.isEmpty()

RhenaudTheLukark added some commits Oct 24, 2017

@bjorn
Copy link
Owner

left a comment

I've tried it out but, but here's out it looks on my side:

selection_225

There's two problems here:

  • The button didn't receive the size it needs for the label to fit properly.
  • I think the button should be centered.

You'll need to set a layout (probably QGridLayout) on the NewTilesetView (which I think would be better named NoTilesetWidget) that will apply the size preferences of the button and to which you can add QSpacerItem instances for centering the button. You could check out noeditorwidget.ui to see how this could be achieved and either do the same in code (see the generated ui_noeditorwidget.h when in doubt) or also make a .ui file for this widget.

explicit NewTilesetView(TilesetDock *parent = nullptr)
: QWidget(parent)
{
QWidget *w = new QWidget(this);

This comment has been minimized.

Copy link
@bjorn

bjorn Oct 26, 2017

Owner

This widget serves no purpose apart from adding just another level to the widget hierarchy.

This comment has been minimized.

Copy link
@RhenaudTheLukark

RhenaudTheLukark Oct 27, 2017

Author Contributor

Alright, I'll take a look at all of that.

I tried to center the button, but everything I tried didn't work.

This comment has been minimized.

Copy link
@bjorn

bjorn Oct 28, 2017

Owner

You basically just need spacer items on all 4 sides for centering the button, and for that it needs to be in a grid layout.

RhenaudTheLukark RhenaudTheLukark
@RhenaudTheLukark

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2017

I tried to center the button, but no matter which way I took the problem from, I couldn't do better than that.

This is a simple task, yet somehow the QGridLayout widget is not taking the whole space the QDockWidget provides, so the button is centered horizontally but not vertically.

I surrounded it with QSpacerItem objects, but I couldn't center it vertically anyway.

Tell me if this is good enough (I really hope it is)

QGridLayout *gridLayout = new QGridLayout(this);

QSpacerItem *spacerVertTop = new QSpacerItem(1, 1, QSizePolicy::Ignored, QSizePolicy::Expanding);
gridLayout->addItem(spacerVertTop, 1, 0, Qt::AlignCenter);

This comment has been minimized.

Copy link
@bjorn

bjorn Nov 7, 2017

Owner

This isn't doing what you think it does. You're actually passing Qt::AlignCenter as the row-span argument to addItem, which explains why the centering isn't working correctly.

I found out that the use of spacers isn't needed at all though, since we can just pass Qt::AlignCenter when adding the New Tileset button. I'll push this change in a moment.

@bjorn

This comment has been minimized.

Copy link
Owner

commented Nov 7, 2017

Tell me if this is good enough (I really hope it is)

Of course it isn't good enough to have a bunch of code sitting there not achieving what it is meant to do. :-)

It wasn't easy for me either to find out what was going on though. A compiler warning would have certainly helped, but in C++ passing an enum value to an int parameter is perfectly valid.

@bjorn bjorn merged commit 77def2b into bjorn:master Nov 7, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@bjorn

This comment has been minimized.

Copy link
Owner

commented Nov 7, 2017

Thanks! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.