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

Templates dock #1612

Merged
merged 8 commits into from Jun 21, 2017

Conversation

@thabetx
Copy link
Contributor

commented Jun 12, 2017

This pull request represents the second checkpoint of the Reusable Object Templates project, it's mainly concerned with creating the dock and managing the created templates.

These are the main items I will focus on doing:

  • Create the templates dock.
  • Display the templates in a tree structure.
  • Load the templates on startup from a predefined path.
  • Create a new template group via a button in the templates dock.
  • Save an object to a specific template group.
  • Load an existing template group.
  • Add a button to the dock to unload a template group.
@bjorn
Copy link
Owner

left a comment

Only a few minor comments. It's looking good and I think you've planned a good next step. I'm looking forward to see the remaining functionality!

QModelIndex ObjectTemplateModel::index(int row, int column,
const QModelIndex &parent) const
{
if (parent == QModelIndex())

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 12, 2017

Owner

Please use if (!parent.isValid())

QModelIndex ObjectTemplateModel::parent(const QModelIndex &index) const
{
if (ObjectTemplate *objectTemplate = toObjectTemplate(index))
return createIndex(0, 0, objectTemplate->templateGroup());

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 12, 2017

Owner

Row 0 can't be always right, you need to determine the index of the template group in mTemplateGroups.

Q_OBJECT

public:
ObjectTemplateModel(const QList<TemplateGroup *> &templateGroups, QObject *parent = 0);

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 12, 2017

Owner

Please use nullptr.

@thabetx thabetx force-pushed the thabetx:templates-dock branch 2 times, most recently from 908f143 to 9ab31dc Jun 12, 2017

thabetx added 2 commits Jun 11, 2017
Create basic templates dock
The dock shows a pre-made template group as a list.

@thabetx thabetx force-pushed the thabetx:templates-dock branch 2 times, most recently from ca91c66 to 01d2b3d Jun 14, 2017

@thabetx

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2017

I've finished loading templates on startup. testing is cumbersome because the file doesn't exist by default.

The file is called templatedocuments.xml and is located in the default data location, this is it's structure

<templatedocuments>
 <templatedocument path="templateGroup1.ttx"/>
 <templatedocument path="templateGroup2.ttx"/>
</templatedocuments>

I think renaming it to templategroups.xml and have internal elements as templategroups might be a good idea but not sure.

To make testing easier I will start implementing the new group button to be able to create groups then will work on adding an object to a loaded group as a template, after that I will work on error handling.

One particular part that I don't like is getting a parent of a template group in the model, I loop through all the documents to find the correct parent.

@bjorn

This comment has been minimized.

Copy link
Owner

commented Jun 14, 2017

I think renaming it to templategroups.xml and have internal elements as templategroups might be a good idea but not sure.

Yes, I think these would be better names as well. The "Document" is only an internal thing referring to the fact that it is a file of which the changes and format are tracked. I'm considering to rename this to "Asset", which is a common name for this as well.

One particular part that I don't like is getting a parent of a template group in the model, I loop through all the documents to find the correct parent.

That's fine.

@bjorn
Copy link
Owner

left a comment

Lots of small comments this time. :-)

private:
QList<ObjectTemplate*> mTemplates;
QVector<SharedTileset> mTilesets;
TemplateGroupFormat *mFormat;
QString mName;
};

typedef QList<TemplateGroup*> TemplateGroups;

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 14, 2017

Owner

Unused now, should be removed.

{
if (!parent.isValid()) {
if (row < mTemplateDocuments.size()) {
return createIndex(row, column, mTemplateDocuments.at(row)->templateGroup());

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 14, 2017

Owner

Coding style: One space too much after return. Also, when all branches of conditions are single lines, we leave out the { and }.

if (row < mTemplateDocuments.size()) {
return createIndex(row, column, mTemplateDocuments.at(row)->templateGroup());
} else {
return QModelIndex();

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 14, 2017

Owner

Line could be removed if the below if is changed to an else if.

QVariant ObjectTemplateModel::data(const QModelIndex &index, int role) const
{
if (!index.isValid())
return QVariant();

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 14, 2017

Owner

These lines can be removed. In general no invalid index should be passed to data, but in case it happens, it will return QVariant() also without this additional check.

writer.setAutoFormattingIndent(1);

writer.writeStartDocument();
writer.writeStartElement(QLatin1String("templateDocuments"));

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 14, 2017

Owner

Should keep the element names lowercase for consistency (also I see the reader requires it to be lowercase :-)).

return false;
}

Format format = mFormat;

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 14, 2017

Owner

I think it sucks to have multiple formats, but since we did the same thing for maps, tilesets and the object types, it only makes sense to do the same thing for the templates since people's games are expected to load these things...

I somewhat wonder if there would be a way to serialize through some kind of interface, with different implementations storing things in different ways (XML, JSON, Lua...). Though that gets even more tricky for loading than for saving. And for sure it can't be done without changing some of these formats. :-(

Maybe we can serialize into a QVariantMap and then save that in the various other formats in some standard way (JSON is easy by using QJsonDocument::fromVariant). It's at least an approach we can try for new files.

const QXmlStreamAttributes atts = reader.attributes();
const QString path(atts.value(QLatin1String("path")).toString());

auto templateGroupFormat = new TtxTemplateGroupFormat();

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 14, 2017

Owner

Hmm, there should be only one instance of TtxTemplateGroupFormat. Like this it is a memory leak. I would suggest to just make a singleton for now, by adding a static method:

TtxTemplateGroupFormat *TtxTemplateGroupFormat::instance()
{
    static TtxTemplateGroupFormat format;
    return &format;
}

This comment has been minimized.

Copy link
@thabetx

thabetx Jun 15, 2017

Author Contributor

This is a new way for implementing a singleton :), I thought of creating the static object as a member and manually checking every time when the instance function is called.

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 15, 2017

Owner

A member is only needed when the object needs to be manually deleted. This static variable is automatically destroyed at some point when the program exists, which I think will not cause any problems.


auto templateGroupFormat = new TtxTemplateGroupFormat();

// TODO: handle errors that might happen while loading

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 14, 2017

Owner

Right, at least don't put nullptr into templateDocuments. :-)

Also, don't forget to initialize templateGroup in MapReaderPrivate::readTemplateGroup with nullptr or you're going to have trouble detecting errors.


if (ObjectTemplate *objectTemplate = toObjectTemplate(index)) {
auto templateGroup = objectTemplate->templateGroup();
// TODO: this doesn't work properly when multiple documents have the same group

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 14, 2017

Owner

This can't happen. You're comparing pointers here and at no point in the code would it insert the same pointer twice.

void Preferences::setTemplateDocuments(const TemplateDocuments &templateDocuments)
{
mTemplateDocuments = templateDocuments;
// This signal isn't used yet

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 14, 2017

Owner

Connect to it in TemplatesDock. :-)

@thabetx thabetx force-pushed the thabetx:templates-dock branch 2 times, most recently from 4523921 to 42709ac Jun 16, 2017

@thabetx

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2017

Currently groups can be created and templates can be saved and viewed in the dock, the workflow still need some refining I will work on it in the upcoming days.

There parts that I'm not sure how to do yet so I left TODOs there.

auto templateGroupDocument = new TemplateGroupDocument(templateGroup, fileName);

if (!templateGroupDocument->save(fileName, error))
return false;

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 19, 2017

Owner

Will leak the above allocated objects. Can use QScopedPointer to prevent leaking (probably only for the templateGroupDocument, given that the document should probably own its template group and delete it in its destructor).

for (const TemplateGroupDocument *templateDocument : templateDocuments) {
writer.writeStartElement(QLatin1String("templatedocument"));

writer.writeAttribute(QLatin1String("path"), templateDocument->fileName());

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 19, 2017

Owner

Please use a relative path here (can check the MapWriter for examples regarding external tilesets and tileset images).

TemplatesView(QWidget *parent = nullptr);

void applyTemplateGroups();
ObjectTemplateModel *objectTemplateModel();

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 19, 2017

Owner

Should be const.

@thabetx thabetx force-pushed the thabetx:templates-dock branch 6 times, most recently from d7cdc0d to 37b0744 Jun 19, 2017

@thabetx

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2017

I've done a punch of improvements

  • The new template dialog behavior is now similar to the new tileset dialog.
  • Insertion now works correctly and the expansion state of the tree will not change.
  • Overwriting a template group will also delete the old file from the tree.
  • Relative paths are used for the stored document.

I've also handled the tilesets so a template group will have no duplicates, this required storing a reference upon saving any template that had a new tileset.

The references then were required to be deleted upon exit which initially violated some assertions. I had to change the destruction order in the main window as the preferences object which stored the documents was deleted after the tileset manager.

@bjorn
Copy link
Owner

left a comment

Alright, I think we're almost ready to merge this one. I've added some minor comments.

while (it.hasNext()) {
if (it.next()->fileName() == fileName) {
int pos = mTemplateDocuments.indexOf(it.value());
delete it.value();

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 20, 2017

Owner

To avoid potential invalid memory access, you should move this delete to after the beginRemoveRows.

QMutableListIterator<TemplateGroupDocument*> it(mTemplateDocuments);
while (it.hasNext()) {
if (it.next()->fileName() == fileName) {
int pos = mTemplateDocuments.indexOf(it.value());

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 20, 2017

Owner

Instead of iterating the list forwards and for each match, finding the index, you could instead iterate the list backwards like for (int i = mTemplateDocuments.size() - 1; i >= 0; --i) {.

}

TemplateGroupDocument::~TemplateGroupDocument()
{
TilesetManager *tilesetManager = TilesetManager::instance();
tilesetManager->removeReferences(mTemplateGroup->tilesets());

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 20, 2017

Owner

If you add references in the TemplateGroup, then please remove them there as well.

@thabetx thabetx force-pushed the thabetx:templates-dock branch 2 times, most recently from b64b0c7 to 899b9d8 Jun 20, 2017

@thabetx thabetx force-pushed the thabetx:templates-dock branch from 234f9b8 to 5c3f7a2 Jun 20, 2017

thabetx added 2 commits Jun 18, 2017
Manage tileset references of template groups
Embedded tileset references are not managed and are currently disabled.
Update saved template groups file format
Update the elements to "templategroup" instead of "templatedocument"

@thabetx thabetx force-pushed the thabetx:templates-dock branch from 5c3f7a2 to 67e39b1 Jun 20, 2017

@bjorn bjorn merged commit 67e39b1 into bjorn:wip/templates Jun 21, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@thabetx

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2017

The last two items in the list will be moved to other PRs.

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