Skip to content

Conversation

@hobyst
Copy link
Contributor

@hobyst hobyst commented Jul 19, 2022

Improves the .gitignore file to ignore all IDE and code editor setting folders regardless of their names and improves the documentation and function interface of LoadDocument() and LoadFontFace() in their file path variants regarding on how relative paths are treated.

The Doxygen comments have been written using the official Markdown compatibility spec of Doxygen, which some IDEs might have issues displaying it the right way due to not implementing Doxygen comments properly. However, by Doxygen standards they are just fine.

  • The docstrings being displayed in Doxygen:
    imagen
    imagen

  • The docstrings being displayed in Visual Studio Code:
    imagen
    imagen

@hobyst
Copy link
Contributor Author

hobyst commented Jul 19, 2022

Emscripten build fails due to compiler warning as error policy in a header file not affected in this pull request:

/home/runner/work/RmlUi/RmlUi/Source/Debugger/../../Include/RmlUi/Core/../Config/../Core/Containers/robin_hood.h:1430:33: error: builtin __has_trivial_copy is deprecated; use __is_trivially_constructible instead [-Werror,-Wdeprecated-builtins]
        Cloner<Table, IsFlat && ROBIN_HOOD_IS_TRIVIALLY_COPYABLE(Node)>()(o, *this);

@hobyst hobyst marked this pull request as draft July 19, 2022 12:58
@hobyst hobyst marked this pull request as ready for review July 19, 2022 18:33
@mikke89
Copy link
Owner

mikke89 commented Jul 19, 2022

Hey, and thanks for the PR!

I'm happy to take the gitignore changes. I also agree that it makes sense to call it file_path in LoadFontFace.

However, the Load... comments are only true if you use the shell from the samples and its platform extensions + custom file interface. These are really only intended to make the included samples work (to find the assets in various situations) and should not be used by clients. Instead, the default file interface will just forward the argument to fopen.

Could you perhaps replace these comments with something like the latter remark?

Just as a side note on the doxygen comments. We don't actually compile our documentation with doxygen so we mostly care for how it looks visually in raw text and secondly for IDEs. For this reason I'd discourage heavy use of markdown but in this case it looks fine though, albeit a bit on the verbose side. For more detailed behavioral comments I'd encourage submitting a PR on the documentation repository instead, there's a handy "edit on github" link on every page.

@hobyst
Copy link
Contributor Author

hobyst commented Jul 22, 2022

However, the Load... comments are only true if you use the shell from the samples and its platform extensions + custom file interface. These are really only intended to make the included samples work (to find the assets in various situations) and should not be used by clients. Instead, the default file interface will just forward the argument to fopen.

Okay so I have delete the whole mention of the search in parent folders as that's just done for the samples right?

Just as a side note on the doxygen comments. We don't actually compile our documentation with doxygen so we mostly care for how it looks visually in raw text and secondly for IDEs. For this reason I'd discourage heavy use of markdown but in this case it looks fine though, albeit a bit on the verbose side. For more detailed behavioral comments I'd encourage submitting a PR on the documentation repository instead, there's a handy "edit on github" link on every page.

If the answer to the question I asked above is affirmative, then deleting the mention of how samples look for their assets will be enough for the comments to look fine again in IDEs and they won't be verbose anymore.

As a sidenote, having some way to build the Doxygen documentation is useful to check the entire API without necessarily having to manually document every single function in the documentation repository. In another PR, I could contribute the Doxygen file I used to generate the documentation in for other devs wanting to check the API reference by just running doxygen Doxyfile in the root folder of the repository.

@mikke89
Copy link
Owner

mikke89 commented Jul 22, 2022

Okay so I have delete the whole mention of the search in parent folders as that's just done for the samples right?

That's right!

If the answer to the question I asked above is affirmative, then deleting the mention of how samples look for their assets will be enough for the comments to look fine again in IDEs and they won't be verbose anymore.

Yup, sounds good!

As a sidenote, having some way to build the Doxygen documentation is useful to check the entire API without necessarily having to manually document every single function in the documentation repository. In another PR, I could contribute the Doxygen file I used to generate the documentation in for other devs wanting to check the API reference by just running doxygen Doxyfile in the root folder of the repository.

I don't really find Doxygen documentation very helpful myself, but if you do then that is all good. As long as the Doxygen file does not require any maintenance I'm happy to take a PR.

@hobyst
Copy link
Contributor Author

hobyst commented Jul 23, 2022

I don't really find Doxygen documentation very helpful myself, but if you do then that is all good. As long as the Doxygen file does not require any maintenance I'm happy to take a PR.

The Doxygen would only need maintenance if you were to change the RmlUi version number (if you want it to appear in the docs) or the folders used to store the includes and the source code files of RmlUi in the top level directory, as I'm referencing Backends, Include, Samples and Source as INPUT directories for Doxygen. But since I enabled RECURSIVE, any folder structure changes inside of those will not need to be reflected inside the Doxyfile.

Also, since Doxygen supports Markdown it would be possible to merge the currently existing documentation files along with everything that Doxygen generates from parsing the source code into a single source for both manually-written docs and the automated API reference. Of course, it would have to be investigated up to what exent such a merge is possible, specially on if it would be possible to port the current theming of the documentation into Doxygen. But since all the documentation Markdown files are in another repository, it might be better just to leave it as it is and use Doxygen just purely for the API reference.

I just changed the commits and deleted the mention of the assets search method used by samples in the comments btw.

Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

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

Thanks for the update. I think the comments can be confusing when using a custom file interface. I made some suggestions which I believe better describes the usage.

@mikke89
Copy link
Owner

mikke89 commented Jul 24, 2022

The Doxygen would only need maintenance if you were to change the RmlUi version number (if you want it to appear in the docs) or the folders used to store the includes and the source code files of RmlUi in the top level directory, as I'm referencing Backends, Include, Samples and Source as INPUT directories for Doxygen. But since I enabled RECURSIVE, any folder structure changes inside of those will not need to be reflected inside the Doxyfile.

Alright, that sounds good to me.

Also, since Doxygen supports Markdown it would be possible to merge the currently existing documentation files along with everything that Doxygen generates from parsing the source code into a single source for both manually-written docs and the automated API reference. Of course, it would have to be investigated up to what exent such a merge is possible, specially on if it would be possible to port the current theming of the documentation into Doxygen. But since all the documentation Markdown files are in another repository, it might be better just to leave it as it is and use Doxygen just purely for the API reference.

Sounds like a huge undertaking. I haven't personally ever been impressed by auto-generated documentation and particularly so by Doxygen. In my opinion our current manually curated documentation is a much better approach.

Improve LoadDocument and LoadFontFace documentation and interface
in their file path variants.
@hobyst
Copy link
Contributor Author

hobyst commented Aug 11, 2022

Just amended my last commit with your suggested changes @mikke89.

@mikke89 mikke89 merged commit af49ec5 into mikke89:master Aug 11, 2022
@mikke89
Copy link
Owner

mikke89 commented Aug 11, 2022

Perfect, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants