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

Refresh assets in collection of Tilesets #1012

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Refresh assets in collection of Tilesets #1012

wants to merge 5 commits into from

Conversation

EdenIndustries
Copy link

No description provided.

@bjorn
Copy link
Member

bjorn commented Jul 1, 2015

Please instead of adding another commit that reverts the previous commit, just remove the commit and force-push to your master branch again. If you need some instructions on how to do this with git just say so.

If you want to open multiple pull requests for independent features, you should create multiple branches for them in your fork.

About the actual changes, could you make sure it follows the same coding style as the existing code? This means mostly:

  • Not indenting with tabs but with 4 spaces
  • Don't put "Get" in front of a getter method (and start them with a lowercase character)
  • Put a space between for and (.

This will make the code easier to read for me.

@@ -53,6 +53,9 @@ using namespace Tiled::Internal;
namespace Tiled {
namespace Internal {

QString mTilesetSourceFiles[MAX_TILESET_SOURCE_FILES];
int mNumTilesetSourceFiles;
Copy link
Member

Choose a reason for hiding this comment

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

I use the m prefix for member variables, but you have just made these ones globals. Were they meant to be member variables?

Also, you could probably just merge these two variables into a single QStringList.

@bjorn
Copy link
Member

bjorn commented Jul 1, 2015

Design-wide, this approach isn't very nice.

There is no need to keep track of the tile source files explicitly in the MapReader, since this information will already be available by just iterating of the Tile instances in the Tileset later.

Also, the TmxMapReader should not go and call into the tileset manager to add these references, because that way we don't know when we can stop watching the files. It also is simply not the job of this class. Instead, the TilesetManager can start watching these files itself when a new tileset is added to it (TilesetManager::addReference) and then it can also stop watching these files once the last reference goes away (in TilesetManager::removeReference).

@bjorn
Copy link
Member

bjorn commented Jul 1, 2015

We'll also need some solution for starting to watch newly added tiles. Possibly some explicit tilesAdded(const QList<Tile *> &tiles) and similar tilesRemoved kind of calls could be made to the TilesetManager from the AddRemoveTiles undo command.

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

4 participants