Skip to content

Templates dock enhancements #1666

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 31, 2017
Merged

Templates dock enhancements #1666

merged 5 commits into from
Jul 31, 2017

Conversation

@thabetx
Copy link
Contributor

@thabetx thabetx commented Jul 27, 2017

This PR adds some convenience features to the templates dock and template saving.

bjorn
bjorn requested changes Jul 30, 2017
View reviewed changes
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.

Nice to see the functionality expanding!

Please fix the typo in "Allow openning an existing template group", and see other comments inline.

src/tiled/objecttemplatemodel.cpp Outdated

bool ObjectTemplateModel::addDocument(TemplateGroupDocument *document)
{
if (!document)
Copy link
Member

Choose a reason for hiding this comment

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

I think this should really be Q_ASSERT(document);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is that? This will only handle the null document in debug mode, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, because it is simply invalid to try adding null document, so it's never expected to happen. The assert would document that and in addition checks for it in debug mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, my understanding now is that we don't need to handle invalid scenarios that won't happen and the Q_Assert helps ensure that while coding.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly.

src/tiled/newtemplatedialog.cpp Outdated
mUi->setupUi(this);

mObjectName = objectName;
mUi->templateNameCheckBox->setEnabled(!objectName.isEmpty());
Copy link
Member

Choose a reason for hiding this comment

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

Why did you choose to have a checkbox? I think it will be enough to auto-fill the field with the name of the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Auto-filling is a nice idea and I won't need the update name function.

src/tiled/mapdocument.h Outdated

bool save(const QString &fileName, QString *error = nullptr) override;
void saveSelectedObject(QString name, int groupIndex);
void saveSelectedObject(const QString &name, const int groupIndex);
Copy link
Member

Choose a reason for hiding this comment

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

Please don't put const on parameters that are passed by value, like groupIndex. It has no relevance in regards to the signature of the function.

src/tiled/newtemplatedialog.cpp Outdated
int index = mUi->groupsComboBox->currentIndex();

okButton->setEnabled(!templateName.isEmpty() && index != -1);
okButton->setDisabled(noTemplateName || index==-1);
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: Missing space around ==

src/tiled/newtemplatedialog.cpp Outdated

Preferences *prefs = Preferences::instance();
QString suggestedFileName = prefs->lastPath(Preferences::TemplateDocumentsFile);
suggestedFileName += QLatin1String("/untitled.ttx");
Copy link
Member

Choose a reason for hiding this comment

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

This should be a translatable string.

src/tiled/newtemplatedialog.cpp
QFileInfo(fileName).path());

mUi->groupsComboBox->addItem(name);
mUi->groupsComboBox->setCurrentIndex(mUi->groupsComboBox->count() - 1);
Copy link
Member

Choose a reason for hiding this comment

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

After this it should call updateOkButton (or connect the index change with this function).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new Qt signal-slot syntax for this signal is a little longer. Calling updateOkButton would be enough in all cases.

src/tiled/newtemplatedialog.ui Outdated
</property>
<property name="checked">
<bool>false</bool>
<string>Create new Group</string>
Copy link
Member

Choose a reason for hiding this comment

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

I think this button should probably use the same "new file" icon as used in the templates dock. This could be its tool tip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice solution, I didn't like how the button was very wide.

src/tiled/newtemplatedialog.h Outdated

public:
explicit NewTemplateDialog(QList<QString> groupNames, QWidget *parent = 0);
explicit NewTemplateDialog(const QList<QString> &groupNames, const QString &objectName, QWidget *parent = 0);
Copy link
Member

Choose a reason for hiding this comment

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

Please use nullptr instead of 0.

src/tiled/newtemplatedialog.cpp Outdated
connect(mUi->createGroupButton, &QPushButton::pressed,
this, &NewTemplateDialog::newTemplateGroup);

mUi->groupsComboBox->addItems(groupNames);
Copy link
Member

Choose a reason for hiding this comment

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

Note that you can just do this:

    auto model = ObjectTemplateModel::instance();
    mUi->groupsComboBox->setModel(model);

This way, there is also no need to update the list of items after adding a new template group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is better, but I had to add a condition to the model that the row is >= 0 so that the -1 initial index doesn't crash.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sounds like a small bug in the QComboBox. :-/

@thabetx thabetx force-pushed the templates-dock-enhancements branch from 3b46d51 to 65c3f76 Compare July 31, 2017 14:57
@thabetx thabetx force-pushed the templates-dock-enhancements branch from 65c3f76 to ca5fcb6 Compare July 31, 2017 16:06
@thabetx
Copy link
Contributor Author

thabetx commented Jul 31, 2017

I updated the PR. I was thinking about adding delete as well but I will work on it while adding detach.

@bjorn bjorn merged commit ca5fcb6 into mapeditor:wip/templates Jul 31, 2017
@bjorn
Copy link
Member

bjorn commented Jul 31, 2017

Cool!

@bjorn bjorn mentioned this pull request Jul 31, 2017
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjorn bjorn bjorn requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@thabetx @bjorn