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

Defaults filename to tileset name or image name #1790

Merged
merged 3 commits into from Oct 26, 2017

Conversation

killerasus
Copy link
Contributor

Solves issue #1783

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.

First of all thanks for looking into addressing this issue!

Since it is a small patch and I found some issues, my suggested change is essentially a complete rewrite. Feel free to just make it though and I will merge this.

This was the first and so far only issue I've labeled as "good first issue" / "hacktoberfest", but feel free to look into anything else that seems doable!

@@ -510,7 +510,16 @@ bool DocumentManager::saveDocumentAs(Document *document)
FormatHelper<TilesetFormat> helper(FileFormat::ReadWrite);
filter = helper.filter();

fileName = getSaveFileName(QCoreApplication::translate("Tiled::Internal::MainWindow", "untitled.tsx"));
QString qName(QLatin1String("untitled.tsx"));
auto tileSetName = tilesetDocument->tileset()->name();
Copy link
Member

Choose a reason for hiding this comment

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

There are some problems with this patch as-is, mostly notably it removes "untitled.tsx" as a translatable string and always tries to translate the suggested file name (which will never work). You also only trim when checking for empty but not when actually using the tileset name, and the naming style of qName, tileSetName and cStrName is off with the rest of the code.

I would implement this as follows:

auto suggestedFileName = tilesetDocument->tileset()->name().trimmed();
if (suggestedFileName.isEmpty())
    suggestedFileName = QCoreApplication::translate("Tiled::Internal::MainWindow", "untitled"));
suggestedFileName.append(QLatin1String(".tsx"));

fileName = getSaveFileName(suggestedFileName);

The above unifies the adding of the extension and reduces the translatable string. The same thing could be done for "untitled.tmx", so there is one less string to translate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bjorn thanks for reviewing my pull request. Would this issue also consider the changes to "untitled.tmx" or should I implement it as another issue?

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 would also consider updating translation files to refer to untitled string instead of untitled.tsx and untitled.tmx, right?

Copy link
Member

Choose a reason for hiding this comment

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

Would this issue also consider the changes to "untitled.tmx" or should I implement it as another issue?

Up to you, I'd be fine either way.

This would also consider updating translation files to refer to untitled string instead of untitled.tsx and untitled.tmx, right?

No, I'm only updating the translation files before a new feature release (usually about 2 weeks in advance). This should be sometime next month.

@bjorn bjorn merged commit 9e273af into mapeditor:master Oct 26, 2017
@bjorn
Copy link
Member

bjorn commented Oct 26, 2017

Thanks!

killerasus added a commit to killerasus/tiled that referenced this pull request Oct 26, 2017
killerasus added a commit to killerasus/tiled that referenced this pull request Oct 26, 2017
killerasus added a commit to killerasus/tiled that referenced this pull request Oct 27, 2017
killerasus added a commit to killerasus/tiled that referenced this pull request Oct 27, 2017
killerasus added a commit to killerasus/tiled that referenced this pull request Oct 27, 2017
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