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

Open file button added when no file opened #1818

Merged
merged 11 commits into from
Nov 15, 2017

Conversation

RhenaudTheLukark
Copy link
Contributor

@RhenaudTheLukark RhenaudTheLukark commented Nov 15, 2017

I created the missing "Open File..." button in the view when no file is opened.

Result in QtCreator:

Result in Tiled:

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

Great addition!

Please see my inline comments, and also please try to clean up the commits on your branch (look into rebasing and rebase your branch to my latest master branch).

@@ -31,9 +32,9 @@ NoEditorWidget::NoEditorWidget(QWidget *parent) :
ui(new Ui::NoEditorWidget)
{
ui->setupUi(this);

Copy link
Member

Choose a reason for hiding this comment

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

Please leave the empty line there. :-)

connect(ui->newMapButton, &QPushButton::clicked, this, &NoEditorWidget::newMap);
connect(ui->newTilesetButton, &QPushButton::clicked, this, &NoEditorWidget::newTileset);
connect(ui->openFileButton, &QPushButton::clicked, this, &NoEditorWidget::openFile);
Copy link
Member

Choose a reason for hiding this comment

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

You can connect to this slot directly without needing NoEditorWidget::openFile:

connect(ui->openFileButton, &QPushButton::clicked, DocumentManager::instance(), &DocumentManager::openFile);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I knew about that one, but I wanted to keep the style of it. I'll follow that way then.

@bjorn bjorn merged commit 26f8d83 into mapeditor:master Nov 15, 2017
@bjorn
Copy link
Member

bjorn commented Nov 15, 2017

Whoops! I didn't notice the failing autobuilds and the change was looking good to me, but actually it didn't compile. Fixed now with change 6fe30ec. :-/

@RhenaudTheLukark
Copy link
Contributor Author

Okay, will update again

@RhenaudTheLukark
Copy link
Contributor Author

RhenaudTheLukark commented Nov 15, 2017

Actually one of the lines I added doesn't work for some reason, specifically the one you told me to add.

It's weird, I swear it used to work before moving to Qt 5.9...

@bjorn
Copy link
Member

bjorn commented Nov 15, 2017

It's weird, I swear it used to work before moving to Qt 5.9...

No, it can't have worked. I wrote that line on GitHub so I hadn't tried to compile it either, but it couldn't work because there are two overloads of DocumentManager::openFile. Hence the cast is required for the connect call to know which one is meant. See the fix I referenced.

@RhenaudTheLukark
Copy link
Contributor Author

I see now, thank you for the enlightenment. It's the first time I see such construction, it seems I still have a lot to learn on C++.

@bjorn
Copy link
Member

bjorn commented Nov 15, 2017

I see now, thank you for the enlightenment. It's the first time I see such construction, it seems I still have a lot to learn on C++.

It's casting to a certain member function signature (a member function of DocumentManager that returns void and takes no arguments). Anyway, even after coding C++ for almost 20 years I too need to look up the syntax for this... It's a lot easier with qOverload (if you ignore its template magic), but that's only available in Qt 5.7 and Tiled's minimum Qt version currently sits at Qt 5.6 (the last Qt version that supports Windows XP).

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.

None yet

2 participants