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.

Sign up for GitHub

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

ImGuiIntegration: Add conversion between ImStrv and StringView #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+40 −3
Open

ImGuiIntegration: Add conversion between ImStrv and StringView #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/Magnum/ImGuiIntegration/Integration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,27 @@ Example usage:
#include "Magnum/ImGuiIntegration/visibility.h" /* defines IMGUI_API */
Copy link
Owner

Choose a reason for hiding this comment

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

Update the docs above to mention the string conversion, please ;)

Also feel free to add yourself to the copyright header in the license block above the file.


#include <imgui.h>
#include <Corrade/Containers/StringView.h>
Copy link
Owner

Choose a reason for hiding this comment

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

This would ideally be also wrapped in #ifdef IMGUI_HAS_IMSTR.

Alternatively I was thinking of deinlining the conversion into a new Integration.cpp file and having just a forward declaration here, but that's maybe taking it a bit too far. The StringView header is not that huge.

#include <Magnum/Types.h>
#include <Magnum/Math/Vector.h>

/* Don't list (useless) Magnum and Math namespaces without anything else */
/* Don't list (useless) Corrade and Magnum and Math namespaces without anything else */
#ifndef DOXYGEN_GENERATING_OUTPUT

/* Currently present only in the features/string_view branch of ImGui */
#ifdef IMGUI_HAS_IMSTR
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
#ifdef IMGUI_HAS_IMSTR
/* Currently present only in the features/string_view branch of ImGui */
#ifdef IMGUI_HAS_IMSTR

... or something like that, to make it clear why the ifdef is there.

namespace Corrade { namespace Containers { namespace Implementation {

template<> struct StringViewConverter<const char, ImStrv> {
static StringView from(const ImStrv& other) {
return StringView{other.Begin, other.length()};
}
static ImStrv to(StringView other) { return ImStrv{other.begin(), other.end()}; }
};

}}}
#endif

namespace Magnum { namespace Math { namespace Implementation {

/* ImVec2 */
Expand Down
25 changes: 23 additions & 2 deletions src/Magnum/ImGuiIntegration/Test/IntegrationTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,40 @@ namespace Magnum { namespace ImGuiIntegration { namespace Test { namespace {
struct IntegrationTest: TestSuite::Tester {
explicit IntegrationTest();

#ifdef IMGUI_HAS_IMSTR
void stringView();
#endif
Comment on lines +38 to +40
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
#ifdef IMGUI_HAS_IMSTR
void stringView();
#endif
#ifdef IMGUI_HAS_IMSTR
void stringView();
#endif

Indent preprocessor same as other code. Same below :)

void vector2();
void vector4();
void color();
void colorLiterals();
};

IntegrationTest::IntegrationTest() {
addTests({&IntegrationTest::vector2,
addTests({
#ifdef IMGUI_HAS_IMSTR
&IntegrationTest::stringView,
#endif
&IntegrationTest::vector2,
&IntegrationTest::vector4,
&IntegrationTest::color,
&IntegrationTest::colorLiterals});
&IntegrationTest::colorLiterals
});
Comment on lines +55 to +56
Copy link
Owner

Choose a reason for hiding this comment

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

Unneeded change.

}

#ifdef IMGUI_HAS_IMSTR
void IntegrationTest::stringView() {
using Containers::StringView;
StringView stringView("Hello World!");
ImStrv imStrv{"Hello World!"};

CORRADE_COMPARE(StringView(imStrv), stringView);

ImStrv c(stringView);
CORRADE_COMPARE(c, stringView);
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, as far as I can see, this would convert c to a StringView and then use Corrade's own equality comparison. Which I don't think is wanted, since it is no different from the other comparison above.

Instead, isn't there something like ImStrCmp that could be used here?

}
#endif

void IntegrationTest::vector2() {
ImVec2 imVec2{1.1f, 1.2f};
Vector2 vec2(1.1f, 1.2f);
Expand Down