s-victor / TinyPedal

Free and Open Source telemetry overlay application for racing simulation
GNU General Public License v3.0
75 stars 8 forks source link

[WIP] adds changes for supporting Imperial units for fuel, temp, and weather #1

Closed thoraxe closed 2 years ago

thoraxe commented 2 years ago

Let me know if you want changes. This is a hacky PR.

s-victor commented 2 years ago

Thanks for the help. Everything looks great. As for string handling (display only), just keep those ":.02f" (or ":.01f", etc) for the final output to label as they were presented in the original file will be good enough, those format string is necessary to keep numbers in acceptable displaying range for widget. Lemme know if you would like change, or I can help change them. Then I'll merge your work to the master branch. And thanks for help.

thoraxe commented 2 years ago

Your original conv_speed function handled string formatting:

https://github.com/s-victor/TinyPedal/blob/d6a38a6ea5314d44f94d14bb5906f78a1da6d5b1/tinypedal/calculation.py#L53-L62

I did the same thing: https://github.com/s-victor/TinyPedal/pull/1/files#diff-da59b650c714234a25bea90621c5ce49a2fd7851c8f056f3a21fa28b9937bd1aR64-R78

The numbers are string-converted and formatted in calculation.py, which means you don't re-apply string formatting inside the method when assigning the text value to the label (which is why you don't see :.02f when setting the label text, as an example).

If you went backwards and made conv_speed output numbers, then you would end up always doing the number-string conversion and formatting when applying the label. As it stands now, it's a mix of both.

While it's a bit of rework for me, IMO you probably want "calculation.py" functions to only handle numbers when doing conversions or mathematics, and return strings when it's drawing stuff. This would mean conv_speed and kpa2psi and rad2deg and maybe others need to change, along with the places that call them.

Up to you -- it's your repo 😊

s-victor commented 2 years ago

Hi, thanks for clarification, and please excuse me, I wasn't looking closely to the code this morning. All is good now, no worries.

I think there is one thing need to adjust though, int(amount_curr) should keep as float rather than int so it keeps more accurate initial value for better accuracy for calculation & displaying.

I'll merge your changes first, then test and see what's necessary to change. And I agree with your that some of the format stuff are scattered around, which doesn't seem to be best. I'll see what I can do to improve later.

Thanks for helping, and I will add your name to contributor list. Lemme know if any issue.

s-victor commented 2 years ago

https://github.com/s-victor/TinyPedal/commit/661f9a56e406b16a4e384a35058cf27400879765 I have just pushed the above commit with mentioned change to amount_curr.

And have moved most of string format stuff from calculation.py to each of individual widget, this should make it more uniformed. Only a few format is left in calculation as necessary.

Also simplify if statement for unit choices and various other code cleanup. And updated docs & contributor list.

If there is no other issue, I can make a new release package for it.

thoraxe commented 2 years ago

@s-victor I just tested with master and it seems fine. Thanks for accepting my changes! I think this is ready for release.