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 --export-tileset command line argument #1872

Merged
merged 4 commits into from Mar 7, 2018

Conversation

Projects
None yet
2 participants
@JoshBramlett
Contributor

JoshBramlett commented Feb 1, 2018

@bjorn Please review. Adding an --export-tileset command line arg has been quite useful to me. Also, please make sure to see the comments at L385. I believe tileset export was recently broken (the "Export..." menu item has the same behavior). Here's a sample tsx to json export to illustrate:

{ "source":"mytileset.tsx",
 "version":1.2
}

@bjorn bjorn self-requested a review Feb 1, 2018

@bjorn

This comment has been minimized.

Owner

bjorn commented Feb 1, 2018

I believe tileset export was recently broken (the "Export..." menu item has the same behavior). Here's a sample tsx to json export to illustrate:

Hmm, that's indeed a bug. Before saving the tileset its filename needs to be temporarily set to empty, because of the following bit of code:

const QString &fileName = tileset.fileName();
if (!fileName.isEmpty()) {
QString source = mMapDir.relativeFilePath(fileName);
tilesetVariant[QLatin1String("source")] = source;
// Tileset is external, so no need to write any of the stuff below
return tilesetVariant;
}

It would be good to change the API here so that such a hacky approach is not necessary.

Regarding this pull request, it's indeed a necessary piece of functionality and it's great that you've worked on adding it. I just hope we can find a way to add it without duplicating so much code, due to the similarities with exporting maps.

@JoshBramlett

This comment has been minimized.

Contributor

JoshBramlett commented Feb 1, 2018

I didn't like the idea of clearing the fileName, but it was the most unobtrusive solution. Plus I figured it would get a conversation going on how to properly fix it.

It would be good to change the API here so that such a hacky approach is not necessary.

There's a Tileset::IsExternal() member function, so we may not need to change the API. However, IsExternal() has the same logic (fileName.isEmpty()), so one solution would be to add a boolean member to explicitly override it. (e.g. mIsExternal || fileName.isEmpty())

The underlying problem seems to be the MapReader impl has state for whether the tileset is external, but the public wrapper sets the fileName regardless.

SharedTileset tileset = readTileset(&file, QFileInfo(fileName).absolutePath());
if (tileset)
tileset->setFileName(fileName);

mReadingExternalTileset = true;
xml.setDevice(device);
if (xml.readNextStartElement() && xml.name() == QLatin1String("tileset"))
tileset = readTileset();
else
xml.raiseError(tr("Not a tileset file."));
mReadingExternalTileset = false;

So the impl could set the mIsExternal Tileset member, and when the converter performs it's tileset.IsExternal() check, it would have correct state. Thoughts?

I just hope we can find a way to add it without duplicating so much code, due to the similarities with exporting maps.

I can clean that up, should be pretty easy.

@bjorn

This comment has been minimized.

Owner

bjorn commented Feb 1, 2018

So the impl could set the mIsExternal Tileset member, and when the converter performs it's tileset.isExternal() check, it would have correct state. Thoughts?

I think it's the wrong approach. The problem is not determining whether the tileset is external or not, it's determining when to write it out in full and when to write out only a reference to a file. The API should be able to export a tileset to a file, external or not. Only when you're writing a map file does it make sense to write an external tileset as a reference.

I can clean that up, should be pretty easy.

Looking forward to it. :-)

@JoshBramlett

This comment has been minimized.

Contributor

JoshBramlett commented Feb 2, 2018

@bjorn Pushed a new commit to address the code duplication.

Regarding the export refactor, I get what you're saying; There's a semantic difference between being an external file and whether the output should have the full definition (i.e. embedded tilesets are not external, yet should still be written in full).

I believe we're now in agreement that the Tileset having state to dictate how it's to be exported isn't the correct way to solve this, but a proper fix would change the scope of the PR pretty dramatically so I think it'd be prudent to break that off into a separate issue. Unfortunately however, that would mean for the time being that clearing the fileName would remain. If you're planning on cutting another release soon I can add the same band-aid to the Tileset GUI export to get that working properly.

Also, what markdown are you using to inline the repo snippets into the comments? I've never seen that before, and I couldn't find anything about it in the docs.

@bjorn

This comment has been minimized.

Owner

bjorn commented Feb 3, 2018

Also, what markdown are you using to inline the repo snippets into the comments? I've never seen that before, and I couldn't find anything about it in the docs.

See Creating a permanent link to a code snippet. I learned this only recently as well. :-)

I'll respond to the rest and review the new version of your patch on Tuesday.

@bjorn

The change looks much better now with the reduced code duplication! I've added some inline comments.

I didn't check, but I'm sure things go wrong when the user tries to use --export-map and --export-tileset at the same time, right? Since they share the filesToOpen() list. It would be good to either fix this or to print an error in this case.

.gitignore Outdated
@@ -54,3 +54,6 @@ util/java/libtiled-java/target

# Linux perf/debugging
gmon.out

# Vim swap files
*.swp

This comment has been minimized.

@bjorn

bjorn Feb 7, 2018

Owner

Please don't add lines for that here. If you want git to ignore vim's swap files, you should add this to your global ignore file (set by core.excludesFile, by default usually at $HOME/.config/git/ignore).

}
}
if (!outputFormat) {
mError = "Format not recognized (see --export-formats)";

This comment has been minimized.

@bjorn

bjorn Feb 7, 2018

Owner

You should make mError a QString and perform the translation here, otherwise lupdate will not be able to recognize that this is a translatable string.

*/
template <typename T>
T *findExportFormat(const QString *filter,
const QString &targetFile);

This comment has been minimized.

@bjorn

bjorn Feb 7, 2018

Owner

Since mError appears to be the only member variable used by this function, I would make that a QString * parameter and take this helper function out of the CommandLineHandler class, just making it a static T *findExportFormat(...).

@JoshBramlett

This comment has been minimized.

Contributor

JoshBramlett commented Feb 27, 2018

@bjorn Apologies for the delay, but I just pushed a commit addressing your latest review.

@bjorn bjorn merged commit 35ffacd into bjorn:master Mar 7, 2018

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.

Owner

bjorn commented Mar 7, 2018

Also, please make sure to see the comments at L385. I believe tileset export was recently broken (the "Export..." menu item has the same behavior). Here's a sample tsx to json export to illustrate:

I've addressed this bug in change d556aaf and removed the workaround you did while merging as ec2615a.

Thanks for your contribution!

@bjorn bjorn referenced this pull request Mar 19, 2018

Open

Bulk Export Tilesets #1161

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment