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

Show Waypoint Type in WaypointInfoWidget #1104

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

Conversation

lordfolken
Copy link
Contributor

2023-01-29-230850_958x258_scrot

@lordfolken
Copy link
Contributor Author

Two questions:

  • i had to add language.hpp in Engine/Waypoints.cpp I'm not sure whether drawing that in there is not violating layering?
  • Potentially the ShortName and Type field should be exchanged for better readability.

@lordfolken lordfolken marked this pull request as draft January 30, 2023 07:38
@lordfolken
Copy link
Contributor Author

Converting to draft. Windows doesn't compile. I have a proper implementation for that, but it ends up with an assertion:
src/Language/Language.cpp Line:91
Epxression language_allowed

@lordfolken
Copy link
Contributor Author

I saw that there was another similar construct in dlgWaypointEdit.cpp will try to refactor.

@MaxKellermann
Copy link
Contributor

  • Engine/Waypoints.cpp I'm not sure whether drawing that in there is not violating layering

The Engine (and any low-level code) should not have anything UI or language related.

@@ -44,3 +45,26 @@ Waypoint::Project(const FlatProjection &projection) noexcept
flat_location_initialised = true;
#endif
}

std::map<Waypoint::Type, tstring> Waypoint::TypeMap = {
Copy link
Contributor

Choose a reason for hiding this comment

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

What could be a simple plain array, initialised at compile-time into .rodata, is now a runtime-initialised std::map with lots of heap allocations, with a complicated lookup procedure.

@@ -108,6 +108,8 @@ WaypointInfoWidget::Prepare(ContainerWindow &parent,
if (!waypoint->comment.empty())
AddMultiLine(waypoint->comment.c_str());

AddReadOnly(_("Waypoint type"), nullptr, Waypoint::TypeMap[waypoint->type].c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

This will crash if the type happens not to be in that std::map.

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