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

Try to keep layer by name in worlds (#2065) #2087

Merged
merged 3 commits into from Jul 11, 2019

Conversation

@JustinZhengBC
Copy link
Contributor

JustinZhengBC commented Mar 18, 2019

I was working on #2065 and I added a method setCurrentLayerByName to MapDocument so that after DocumentEditor switches documents within a world, it can try to also change the layer to one with an identical name, as the original issue requested.

I was not able to reproduce the second request in the original issue. Changing the active tileset already persists between maps for me.

Copy link
Owner

bjorn left a comment

Thanks for working on this! I had a few comments on the implementation which I've written inline.

In general, I think when we go and try keeping the currently selected layer the same, we maybe should do the same for all selected layers.

src/tiled/documentmanager.cpp Outdated Show resolved Hide resolved
src/tiled/documentmanager.cpp Outdated Show resolved Hide resolved
src/tiled/mapdocument.cpp Outdated Show resolved Hide resolved
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Mar 19, 2019

I was not able to reproduce the second request in the original issue. Changing the active tileset already persists between maps for me.

I think it will only persist if both maps are referencing the same tileset (so it must be an external tileset). If they both embed a tileset which is similar (potentially based on the same tileset image), then it will not work. To make that work, we could remember the previously selected tileset and then use Tileset::findSimilarTileset to find a similar one among the tilesets shown for the newly selected map.

@JustinZhengBC

This comment has been minimized.

Copy link
Contributor Author

JustinZhengBC commented Mar 21, 2019

@bjorn I've made the requested changes and I was also able to replicate the tileset issue after embedding the tilesets

@bjorn bjorn self-requested a review May 18, 2019
@bjorn bjorn force-pushed the JustinZhengBC:BUG-2065 branch from 954e77e to 396c226 May 28, 2019
@bjorn
bjorn approved these changes May 28, 2019
Copy link
Owner

bjorn left a comment

I did some testing and found several issues, and I've pushed a commit which I think fixes all of them. Maybe you could have a go at it as well, @JustinZhengBC, and let me know whether you're fine with merging this as-is?

Note that I force-pushed to your BUG-2065 branch since I've squashed your first two commits in one and reworded the commit messages a bit. So be sure to reset your local branch to the remote after doing a "fetch" instead of trying to just "pull".

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 9, 2019

@JustinZhengBC Do you think you'll have time to review and test the functionality after my changes above? Maybe we can merge this change this week.

@JustinZhengBC

This comment has been minimized.

Copy link
Contributor Author

JustinZhengBC commented Jul 9, 2019

@bjorn yes, I can definitely find time to test the changes in the next few days.

@JustinZhengBC

This comment has been minimized.

Copy link
Contributor Author

JustinZhengBC commented Jul 10, 2019

@bjorn I got a chance to try out your changes, and I can confirm your changes fix the additional test cases.

@bjorn bjorn force-pushed the JustinZhengBC:BUG-2065 branch from 396c226 to 61d8a3e Jul 11, 2019
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 11, 2019

Hmm, there's still a bug, or potentially an issue introduced in my latest change, which breaks using Ctrl to select multiple layers in the Layers view.

* Selecting the same tileset didn't work, because it was happening
  before the switch to the other map, at which point tilesets embedded
  in the other map are actually not shown yet in the Tilesets view,
  so they can't be selected.

* Synchronizing possible multiple selected layers was broken, because
  once the switch to the other map happened, the Layers view would
  cause a selection change that reset the selected layers to just the
  current one.

* Fixed a few potential crashes, like when switching away from a map
  without any layers or without any tilesets.
@bjorn bjorn force-pushed the JustinZhengBC:BUG-2065 branch from 61d8a3e to f2319bb Jul 11, 2019
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 11, 2019

Alright, I think I got it all working now... but this could use some more testing. I'll merge it now, probably will do a new development snapshot next week.

@bjorn bjorn merged commit 4455201 into bjorn:master Jul 11, 2019
1 of 3 checks passed
1 of 3 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
WIP Ready for review
Details
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 11, 2019

Thanks again for your work on this, @JustinZhengBC!

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.