SeedSigner / seedsigner

Use an air-gapped Raspberry Pi Zero to sign for Bitcoin transactions! (and do other cool stuff)
MIT License
699 stars 161 forks source link

Fix extension in font not found exception message #471

Closed dbast closed 1 year ago

dbast commented 1 year ago

The exception had the .ttf extension hardcoded, which lead to error messages like seedsigner-icons.ttf not found, which is missleading as there only is a seedsigner-icons.otf file. Also add the full font file path to the exception to know where the file is actually expected.

This was found while working on https://github.com/SeedSigner/seedsigner/pull/469 before the font setup was correct in the PR.

jdlcdl commented 1 year ago

@dbast,

Thank you for your recent work.

In case you are not yet aware, we've had an informal code-freeze for the past 3 weeks, opting only for most critical bug fixes, as release 0.7.0 is imminent. Others will likely review your detailed improvements after 0.7.0 is out-the-door.

If interested, all are welcome to join the "SeedSigner New Devs" telegram group here

jdlcdl commented 1 year ago

I have no problem with these changes after reviewing with my own eyes.

Could you help me to reproduce the same errors, so that I can see them myself without your changes, and then verify that they're resolved with this pr? Or is it the case that these only show up during automated runs on github?

kdmukai commented 1 year ago

I don't feel too strongly about this, but I slightly prefer just fixing the font file name (which is obv a bug/inaccurate) in the exception but not including the full path (unlikely to be useful now that the niche CI issue has been resolved, won't fit in the onscreen exception handler display).

dbast commented 1 year ago

Ok. Reduced the PR to only fix the extension in the Exception message.

This can be tested by making the folder src/seedsigner/fonts unavailable and e.g. running the tests.

jdlcdl commented 1 year ago

I made ./src/seedsigner/resources/fonts unreadable and indeed provoked the ".ttf" font-file error.

Tested that with this pr, the font file extension is now correct.

newtonick commented 1 year ago

ACK