Skip to content

Converted to alire project #1

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

michael-hardeman
Copy link

I have converted this to an alire project ready for you to publish to the alire crate store!

Thank you for having such a good quality base for me to work off of. I think this repo can help others as well. I had an excellent experience moving the codebase here to work in the alire build system, it only took a few hours to get everything working. This might possibly be the future for Ada.

Here is a demonstration of the example project running on my machine:
working

Converting to an Alire published crate will make this project
easier to build and publish. Since it only depends on 1 already
existing published alire crate, it seem logical to pull that
dependency from alire, and publish this to it to make it more widely
available.

Currently the libraries build, but the example project is still a WIP.
I moved the main code from imgui into a subfolder of the same name then
moved the example code out of the test_project subfolder and into a
sibling package.

I have set the sibling up to depend on the parent. I can build both now.
I'm not entirely sure if I've done this 100% correctly, but I can get
the example app to run.
 * Made the example follow the Alire Style Guides
 * Removed unessisary Float type
 * Improved clarity of code by removing use statements
 * Added option to toggle light theme in File menu
 * Improved naming clarity
@Cre8or
Copy link
Owner

Cre8or commented Aug 25, 2023

Thank you for the PR! To be honest, I wasn't expecting any traffic here this early in development, which is why the repo is still very bare-bones (lack of a proper readme, license, etc). It's also why I haven't considered moving it to Alire yet.

While I'm glad to hear you had little to no trouble getting the library (and test project) to run on your machine, I don't think it's ready for a crate release. There are still many features missing, the existing ones may need to be moved/renamed, and I'm not even sure if the current package hierarchy is any good. As such I'd prefer to avoid letting people set up potentially unstable dependencies on my library. In fact, everything present in this library comes directly, and exclusively, from my personal use case - which is definitely not suited for everyone! And since I'm still not 100% comfortable with Ada, things may change a lot going forward.

With that aside, here are some of my unsorted thoughts regarding your remaining changes:

  • I'm not particularily fond of the alire code style. In fact, even the "intended" style used in the RM (and the standard libraries) looks... difficult to read, in my opinion. Now, I'm aware this is an inherently subjective matter, but I'd prefer to stick with my code style - including the comments, numbers of blank lines, etc. It's not for everyone! But it works for me, and that's why I'm using it.
  • The folder structure, I'm impartial on. I like that the library is now nested in its own folder, which seems particularily useful for easier navigation on Github, but it means that project dependency paths will become longer (and navigating the folders on my local machine will become slightly more annoying 😛).
  • The custom float type in the example file is intended to help highlight the fact that the binding is generic, and can either work with the standard Float type, or a custom one. It doesn't serve a purpose other than that, but I think that's to be expected from an example project.
  • The elaboration pragmas are indeed not required, strictly speaking, but good practice to have. It helps detect (and prevent) elaboration issues at an early stage, and is often the only way to ensure package data is properly initialised before it is accessed (see "Access Before Elaboration", or ABE problems). As such, removing them is a bad idea.
  • I like the idea of adding a theme selection menu item. My only suggestion is to use the Menu_Item_Toggle function (instead of the procedure), as it returns true when the item has been activated (think "pulse"), allowing you to react to the user clicking it, and thus making Theme_Is_Light unnecessary.

I'm a little short on time these days, but when that changes I want to take a proper look at this, and amend the things this repo sorely lacks (readme, license, links to references, etc.). Nonetheless, thank you for your feedback!

* Using function version of Menu_Item_Toggle removes a boolean state
  flag
* Fixing erroenous placement of End_Window for collapsable window being
  inside the if statement which caused a crash when collapsing the
window. I'm not sure how it got there.
* Removed use Ada.Text_IO so where the Put_Line function comes from is
  equally obvious to an example user.
@michael-hardeman
Copy link
Author

michael-hardeman commented Aug 25, 2023

Thanks for taking the time to review the changes.

I myself have been exploring adding the docking variant of the imgui library to the project and I have already seen the Style parts the API only have thin binding support at the moment and I'm sure many other areas are similar. I understand why you feel it's not ready and that is fine, I do think the conversion to an Alire crate change can still be considered as doing so does not mean we must push it immediately and does grant us some definite development benefits such as:

  • standardizing the structure of our project, tests, and dependencies
  • allowing the developers/users to pull the OpenGLAda dependency more easily
  • Publishing will require no additional work when the time is right

As to your response bullet points:

  1. I'm not particularily fond of the alire code style. In fact, even the "intended" style used in the RM (and the standard libraries) looks... difficult to read, in my opinion...

I noticed you had a distinct code style in your library. I tried to leave that untouched as it's your library and your style. I figured it might be important to you and aid you in the further development of the library. I did, however, think the example should be of incredibly common RM style since the example is for others and is what people will copy to start a new project. Of course it's your project and so you get to choose :)

As a side note I think there are definite benefits to automated style checkers and I like that Alire enables that by default, Perhaps we can look into configuring those style checks to match what you like?

  1. The folder structure, I'm impartial on...

I think the benefits of using the Alire mono-repo project structure outweighs any benefits derived from having the example folder where it originally was. This is my first time using Alire so I might just be a huge noob and there is a way to keep things comfortable for you and still work with it.

  1. The custom float type in the example file is intended to help highlight the fact that the binding is generic...

I figured that was the case and I do like having things in the example project purely for example purposes. I was, however, wondering why the binding needed to be generic? What other float types might people use here? If there is a good reason for it to be generic on float types then we might consider providing a default implementation for floats and double floats like how Ada.Numerics.Generic_Elementary_Functions has default Ada.Numerics.Elementary_Functions for the standard Float type. Non generic equivalents of Numerics.Generic_Elementary_Functions for each of the other predefined floating point types are defined similarly, with the names Numerics.Short_Elementary_Functions, Numerics.Long_Elementary_Functions, etc.

  1. The elaboration pragmas are indeed not required, strictly speaking, but good practice to have...

I'm not familiar with this practice. I will read about it more, but my inital reaction is it seems like forcing elaboration would actually hide the issues? It seems like if there are elaboration issues and we remove the Elaborate_All () then those issues would re-appear?

  1. My only suggestion is to use the Menu_Item_Toggle function...

The thing that prompted me to add one was seeing your commented out usage of one of the theming functions. I figured you were testing it out and wanted something like it :)

Excellent suggestion on the function variant. It worked perfected in my testing and cleans that code up nicely.

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