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

MapEditorController: Show details for map part operations #2110

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

Conversation

dl3sdo
Copy link
Member

@dl3sdo dl3sdo commented Nov 14, 2022

When merging or removing a map part the user is not informed about the number of affected objects.
This easy change tells the user how many objects are subject of merging or removing a map part.

When merging or removing a map part the user is not informed about
the number of affected objects.
This easy change tells the user how many objects are subject of merging
or removing a map part.
@dl3sdo
Copy link
Member Author

dl3sdo commented Nov 14, 2022

Examples:
MergeMapPart
RemoveMapPart
RemoveEmptyMapPart

@pkturner
Copy link
Contributor

I like this, considering the whole.

As for the details, I would prefer not to be questioned about removing an empty map part.

@dl3sdo
Copy link
Member Author

dl3sdo commented Nov 19, 2022

@pkturner: Indeed it's an option to remove the popup window if empty map parts shall be removed or merged.
I added another commit so that there is the option to take both or pick the first one only.
@lpechacek,@dg0yt: what is your preference?

@lpechacek
Copy link
Member

@lpechacek,@dg0yt: what is your preference?

Given that the map part operations can be undone, I'm all fine with skipping the confirmation on empty parts.

Code-wise, I'm going to review the second patch soon. I've got a remark right away though. The variable holding the number of map parts (i) appears in more places after the change. It should get a more descriptive name.

@dl3sdo
Copy link
Member Author

dl3sdo commented Nov 20, 2022

@lpechacek,@dg0yt: what is your preference?

Given that the map part operations can be undone, I'm all fine with skipping the confirmation on empty parts.

Code-wise, I'm going to review the second patch soon. I've got a remark right away though. The variable holding the number of map parts (i) appears in more places after the change. It should get a more descriptive name.

I agree and will change i to num_objects once your review is finished.

Copy link
Member

@lpechacek lpechacek left a comment

Choose a reason for hiding this comment

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

LGTM. The comment about the num_objects change is more a nitpicking than a code issue. Take it and re-rest the change if you like the change, or leave it as is. Thanks!

src/gui/map/map_editor.cpp Outdated Show resolved Hide resolved
If the map part to be removed or merged is empty, it's an option to
avoid asking the user before removing the empty map part as no objects
are affected.
This commit removes the popup window in case of empty map parts.
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.

3 participants