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

MeasureWidget: Warn about min size in combined symbols #2084

Conversation

lpechacek
Copy link
Member

This enhancement is especially useful when working with @mlerjen's "halo" symbol set (mentioned multiple times, e.g. in #1004). Please consider merging.

@lpechacek
Copy link
Member Author

@mlerjen's "halo" symbol set (mentioned multiple times, e.g. in #1004)

I've just realized that it's actually the symbol set contributed in #2032.

Copy link
Member

@dl3sdo dl3sdo left a comment

Choose a reason for hiding this comment

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

Libor, thank you for considering combined area symbols. Your commit works fine except in one case: if a new symbol part is just added to the combined symbol without specifying its type, Mapper will crash due to dereferencing a nullptr. Adding an additional check solves this issue:
if (part) { if (part->getType() == Symbol::Area) ........ }
See dl3sdo@c1b9ed8

Combined symbols may contain areas with a minimum size set, be it other
area symbols or private sub-symbols. This patch makes the measure tool
examine the embedded symbols for minimum size and warn if the object is
smaller than any of the minimum sizes.

Closes OpenOrienteeringGH-2083.

Co-authored-by: Matthias Kühlewein <[email protected]>
@lpechacek
Copy link
Member Author

Libor, thank you for considering combined area symbols. Your commit works fine except in one case: if a new symbol part is just added to the combined symbol without specifying its type, Mapper will crash due to dereferencing a nullptr. Adding an additional check solves this issue: if (part) { if (part->getType() == Symbol::Area) ........ }

Thanks, Matthias, for testing the commit and the fix! I agree with the approach, however, I'd avoid the additional indentation level induced by if (part) ... but rather skip the part in the loop with a continue. Otherwise, I agree with all the changes and I've merged them into my commit.

@lpechacek lpechacek force-pushed the issue-2083-combsym-area-size branch from a083701 to 2a551a4 Compare September 10, 2022 17:37
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.

2 participants