Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix extension in font not found exception message #471

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

dbast
Copy link
Contributor

@dbast dbast commented Sep 4, 2023

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 #469 before the font setup was correct in the PR.

@jdlcdl
Copy link

jdlcdl commented Sep 4, 2023

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

jdlcdl commented Sep 4, 2023

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

kdmukai commented Sep 4, 2023

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).

The exception had the `.ttf` extension hardcoded, which lead to error
messages like `seedsigner-icons.ttf` not found. That is missleading as
there only is a `seedsigner-icons.otf` file.
@dbast dbast changed the title Fix font not found exception and add full path to exception Fix extension in font not found exception message Sep 4, 2023
@dbast
Copy link
Contributor Author

dbast commented Sep 4, 2023

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

jdlcdl commented Sep 4, 2023

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

ACK

@newtonick newtonick merged commit 16cb15e into SeedSigner:dev Sep 14, 2023
@dbast dbast deleted the font_exception branch September 14, 2023 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged Not Yet Released
Development

Successfully merging this pull request may close these issues.

4 participants