enkisoftware / imgui_markdown

Markdown for Dear ImGui
zlib License
1.03k stars 69 forks source link

Code organization / structure #5

Closed dougbinks closed 3 years ago

dougbinks commented 5 years ago

@bkaradzic mentions in issue 4 that he would prefer a separate header for the external interface.

The way we use imgui_markdown.h in our project is we expose an external interface via our own string container, and the config isn't exposed. I'd thought people would likely use it that way, but I can see that for library projects they would like to expose a full external interface.

I think there are several alternatives here:

  1. Keep the code as it is.
  2. Stick to one header file, but add #ifdef to separate the external interface and declarations from implementation, so that you could create a header and source file via these.
  3. Split into two as per https://github.com/bkaradzic/bgfx/blob/master/3rdparty/dear-imgui/widgets/markdown.h
  4. Split into header and .cpp file.

Thoughts?

bkaradzic commented 5 years ago

Aren't 3. and 4. the same thing?

dougbinks commented 5 years ago

Somewhat, but 3 could also be two headers.

bkaradzic commented 5 years ago

If you mean two headers because of name, I renamed it (.h / .inl) to match the rest of the widgets in my repo.

This also matches ImGui way of extending it via imgui_user.* files:

https://github.com/bkaradzic/bgfx/blob/06e8c2e7ed83f230b21596be2333dc4ee66f85b6/3rdparty/dear-imgui/imgui_user.h#L50

https://github.com/bkaradzic/bgfx/blob/06e8c2e7ed83f230b21596be2333dc4ee66f85b6/3rdparty/dear-imgui/imgui_user.inl#L79

Another example of this organization: https://github.com/CedricGuillemet/ImGuizmo

ocornut commented 5 years ago

The imgui_user.* mechanism is not necessary nowadays. It only made sense at the time where this was the only way to access internals, but now we have imgui_internal.h so I wouldn’t recommend using it although I kept the ifdef/include for backward compatibility purpose.

The only benefit is that end user can include imgui.h and you can include your own declarations in there, but it would be equally sane and more explicit if the user had to include their own .h with the extensions.

bkaradzic commented 5 years ago

@ocornut I thought it's there so that extension could use ImGui internals without exposing them into headers.

ocornut commented 5 years ago

@ocornut I thought it's there so that extension could use ImGui internals without exposing them into headers.

Well that's equivalent to my_extension.cpp doing an #include "imgui_internal.h, it won't leak to the user which should only see my_extension.h.

dougbinks commented 3 years ago

If imgui_markdown gets significantly more complex then we'll split the code, but for now we'll leave as is.