29jm / SnowflakeOS

"It is very special"
https://jmnl.xyz
MIT License
316 stars 18 forks source link

Added basic integer calculator functionality. #21

Closed the-grue closed 3 years ago

the-grue commented 3 years ago

A couple comments:

* spaces after ifs and fors (there's a [style guide](https://github.com/29jm/SnowflakeOS/blob/master/CONTRIBUTING.md) of sorts now, but those are flexible)

I'll fix these and add to the PR. It'll take me a bit of practice to hit all the style guidelines as some differ for how I naturally code. I remembered to remove all the tabs this time.

* not a huge fan of keeping `#ifdefs DEBUG` in live code for readability, although I know how useful they are when debugging; your call

I left them in for now as I may wish to do more work on the app. I can easily remove them though, it seems the logic is working fine for those parts.

* Have you considered something like `strtol` in `accumulate_selection`? It's a pretty recent addition to this libc but it's there if needed ^^ your way obviously works too.

I didn't. I went looking for ltoa and found itoa but that was after I had already implemented accumulate_selection. I'll revisit and see if strtol can easily drop-in replace my code.

Some design thoughts on handling buttons' buffers and how those are exposed to the user of the library: the idea is that a button owns its text buffer, at least in the sense that it's its job to free it when the button itself is destroyed (which happens when the container it's in is cleared, e.g. in lbox_clear). Replacing it by hand and keeping a reference to it is error prone, because while in this case it's fine as the button will never get destroyed, in some other case the button would end up calling free on a static buffer, which is undefined behavior. What's needed here is a function like button_set_text that handles freeing the previous text and strduping the new one. Doing things the old way would still be possible when convenient, but it'd be more obvious that it's not the "right" way and that caution is needed.

I agree, my solution to this is a little hackish, however I was unable to get the button_set_text to work and I would get memory errors when trying to use it with anything other than a static string ("0" for example). Passing buf wasn't working and would throw an error. strdup would also throw an error. I found that even trying to read button->text would throw an error. I had some concerns there was some sort of translation error with addresses between user space and kernel space. I'll revisit it and put in a little more time to try to come up with a better solution.

I appreciate the feedback!

Jim

29jm commented 3 years ago

I'll fix these and add to the PR. It'll take me a bit of practice to hit all the style guidelines as some differ for how I naturally code. I remembered to remove all the tabs this time.

I left them in for now as I may wish to do more work on the app. I can easily remove them though, it seems the logic is working fine for those parts.

I didn't. I went looking for ltoa and found itoa but that was after I had already implemented accumulate_selection. I'll revisit and see if strtol can easily drop-in replace my code.

Alright with me!

I agree, my solution to this is a little hackish, however I was unable to get the button_set_text to work and I would get memory errors when trying to use it with anything other than a static string ("0" for example). Passing buf wasn't working and would throw an error. strdup would also throw an error. I found that even trying to read button->text would throw an error. I had some concerns there was some sort of translation error with addresses between user space and kernel space. I'll revisit it and put in a little more time to try to come up with a better solution.

Those crashes are due to my clumsiness I'm afraid. In the main function, text_field is redefined, so the outer text_field used everywhere else is never assigned anything... Removing that redeclaration will make it behave.

Also I had entirely forgotten the existence of button_set_text, in fact I remembered wanting to write it and not doing it ^^

29jm commented 3 years ago

I've added clang-format support (here) fwiw, hopefully it can make style-related things less boring: you can write code the style you prefer, and then only before committing making it use this project's style in one command. It doesn't check every little thing but it seems to be more than enough.

the-grue commented 3 years ago

@29jm I believe I have corrected all defects outlined above as requested. Please give it a look.

29jm commented 3 years ago

That looks great! Thanks again

29jm commented 3 years ago

Is there something you wished was done differently, or existed at all, in the ui library (or elsewhere)? I haven't been able to pinpoint what the next "highest impact" improvement to this library could be.