-
Notifications
You must be signed in to change notification settings - Fork 45
Added Support for Multiple Batteries #63
base: master
Are you sure you want to change the base?
Conversation
Otherwise they are treated as spacers
dwzg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what you did there. The ability to modify the label is quite nice, altough it might be confusing for the user, if the label that is set in the config would just be ignored or changed.
In my opinion, it would be good, if there was a possibility to deactivate the usage of the model name as the label. I only have a single battery to show and always seeing the part number of this battery in paleofetch is not so nice for me.
I think in general, having the ability to pass function parameters in the config would be a big improvement. This would remove the duplicate functions for n-th device like for battery or gpu.
paleofetch.c
Outdated
| } | ||
|
|
||
| // get model name of battery | ||
| char *model_name = malloc(30); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this allocated memory freed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I hadn't thought about this. Since not every field's label is dynamically allocated, the label's memory cannot be freed indiscriminately after output like we do with the values. Do you have any suggestions on how we could free the memory cleanly, without just adding special cases for all of the functions that do dynamically allocate memory?
paleofetch.c
Outdated
| if (strcmp(label, "") != 0) { // check if label is empty, otherwise it's a spacer | ||
| ++offset; // print next line of information | ||
| if (strcmp(c.label, "SPACER") != 0) { // check if label is SPACER, otherwise not a spacer | ||
| ++offset; // print next line of informati |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you dropped this: on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll fix that right now.
I wasn't sure about what the best way to handle this was. One way to fix this is to treat the "name" label as a format string, and then use that format string with the model name as an argument to determine the final label. Does this sound like a good idea?
See the above
That's a great idea, and it would definitely make the code a lot less redundant. |
|
I went ahead and implemented the format string approach. |
I made each function take in a pointer to the label so that they can modify them. This was necessary so that each battery could be given the correct name.
I went fairly in-depth in my issue.
There is a little bit of buffer micromanagement, but it was a really easy optimization to make so I just went for it.