Add --export-tileset command line argument

[JoshBramlett]

@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]

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:

https://github.com/bjorn/tiled/blob/a399d4886df683a3b2bdc2d72b41a570e56900a3/src/libtiled/maptovariantconverter.cpp#L134-L141

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]

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.

https://github.com/bjorn/tiled/blob/a399d4886df683a3b2bdc2d72b41a570e56900a3/src/libtiled/mapreader.cpp#L1311-L1313

https://github.com/bjorn/tiled/blob/a399d4886df683a3b2bdc2d72b41a570e56900a3/src/libtiled/mapreader.cpp#L174-L183

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]

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]

@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]

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.

[bjorn]

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).

[bjorn]

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.

[bjorn]

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]

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

[bjorn]

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!