Skip to content
This repository was archived by the owner on Apr 28, 2021. It is now read-only.

Conversation

@srithon
Copy link
Contributor

@srithon srithon commented May 13, 2020

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.

Copy link
Contributor

@dwzg dwzg left a 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);
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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

Copy link
Contributor Author

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.

@srithon
Copy link
Contributor Author

srithon commented May 19, 2020

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.

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.
This addresses your next point. With this change, you would be able to have Battery: as your defined label, and have that as the final output in the end as well. If a user wanted to have the model name of the battery as the label, they could use %s: as their defined label in the config.
Since the format string is determined at compile-time, it would not pose a vulnerability.

Does this sound like a good idea?

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.

See the above

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.

That's a great idea, and it would definitely make the code a lot less redundant.
For now, we only need one argument max per function, so we would be able to get away with just adding a 4th "param" field to the config struct.
However, we may need a better solution than this if we end up having more than 1 argument per function.

@srithon
Copy link
Contributor Author

srithon commented May 19, 2020

I went ahead and implemented the format string approach.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants