Closed FaBjE closed 1 month ago
This is awesome!!!
The checks are failing because you should specify exactly which type of error you're expecting. It should be as specific as possible, otherwise you risk catching and ignoring the wrong error which is typically catastrophic.
What you want should look something like:
try:
...
except ValueError as e:
if str(e) != "the font is upside down":
# Unexpected error, reraise
raise e
You should replace ValueError
with the exact class of the error you're trying to suppress.
I don't seem to get any exceptions here, so I don't know what you're encountering.
It looks like QFontDatabase.addApplicationFont
returns a font_id
of -1
when a font could not be loaded. It may be good to explicitly handle that.
Also, could you please refactor things a bit to keep FontStyle.__init__
clean? I'm thinking something along the lines of:
for font_path in get_available_fonts():
item = make_combobox_item_for_font(font_path)
if item is not None:
fonts_model.appendRow(item)
...
def make_combobox_item_for_font(font_path: Path) -> QStandardItem | None:
...
Note that the new-style Class | None
annotation requires
from __future__ import annotations
at the top of the file.
Thanks @FaBjE! I think this should be straightforward to merge.
To fix
E TypeError: unsupported operand type(s) for |: 'PyQt6.sip.wrappertype' and 'NoneType'
you just need to add
from __future__ import annotations
at the top of the file.
Thanks @FaBjE! I think this should be straightforward to merge.
To fix
E TypeError: unsupported operand type(s) for |: 'PyQt6.sip.wrappertype' and 'NoneType'
you just need to add
from __future__ import annotations
at the top of the file.
If I add that, I get two ruff errors:
- exit code: 1
src/labelle/gui/q_label_widgets.py:127:32: UP007 Use `X | Y` for type annotations
src/labelle/gui/q_label_widgets.py:161:63: UP007 Use `X | Y` for type annotations
Found 2 errors.
And a mypy error
AssertionError: Cannot find module for __future__.annotations
If I add that, I get two ruff errors
Ah, that's because Optional[SomeClass]
is equivalent to the preferred SomeClass | None
. Ruff doesn't fix this automatically because it's considered an "unsafe fix". But I'll enable that, because it's still very safe, and I think it'll make things easier.
And a mypy error
That's pretty weird, I'm not able to reproduce that from my side.
Why don't you give it another shot by removing Optional
, but let me know if you're fed up and I'll take care of it for you.
it's considered an "unsafe fix". But I'll enable that
Done in #59
Feel free to merge main
to take advantage of it.
Looks great, thanks @FaBjE!
One more thought, it looks like we are dropping fonts whenever they aren't accepted by PyQt. We are rendering the fonts with Pillow, which is different. So PyQt being unable to load a font doesn't necessarily mean Pillow can't load it. I wonder if dropping these missing fonts in the GUI could create confusion? Would it make sense to instead add a non-stylized combo-box entry in this case?
Feel free to merge
main
to take advantage of it.
It passes now. Take a good look at the last commit. I have no clue if the automatic changes are correct.
It passes now. Take a good look at the last commit. I have no clue if the automatic changes are correct.
Yes, that's correct.
Ah, wait, I think I'm a bit confused...
Under which circumstance can make_combobox_item_for_font
return None
? The only return in the method is return item
at the end. Right before the return you call item.setFont
. But None
doesn't have .setFont
, so I think that it's impossible to return None
?
Looks great, thanks @FaBjE!
One more thought, it looks like we are dropping fonts whenever they aren't accepted by PyQt. We are rendering the fonts with Pillow, which is different. So PyQt being unable to load a font doesn't necessarily mean Pillow can't load it. I wonder if dropping these missing fonts in the GUI could create confusion? Would it make sense to instead add a non-stylized combo-box entry in this case?
My goal was to just show the font name in the default font in case PyQt doesn´t accept it. As far as I can see I do this. Am I missing something? https://github.com/labelle-org/labelle/pull/56/files#diff-f4c561fb566f9f5206e2dd8c35ae12a6f4c55e71f0aae92b3faabdaa0dc81376R75
"If font loads correctly, apply font (family) name". Otherwise it just continues with the bold thingy?
Ah, wait, I think I'm a bit confused...
Under which circumstance can
make_combobox_item_for_font
returnNone
? The only return in the method isreturn item
at the end. Right before the return you callitem.setFont
. ButNone
doesn't have.setFont
, so I think that it's impossible to returnNone
?
Ah yes. I just copied your function definition. I think it should always return an item. Maybe remove the "none" option?
I think it should always return an item. Maybe remove the "none" option?
Ya, that's exactly what I'm thinking.
And also remove if item is not None
.
Just a tiny thing I cooked up today. When selecting a font I got a big list of names, but no clue what they looked like.
This PR changes the combo-box so that every font name is shown in the font itself. Because we don´t rely on system fonts but use our own font directory the handling is a bit "custom" instead of using a standard font selection combo-box. But for this purpose I think it works very well.
Picture:![image](https://github.com/labelle-org/labelle/assets/5355001/4bcf92b4-f997-43c1-a6f4-9a5965957410)
Please advise how to handle the empty excerpt error. I added them because the font handling may fail. But it it is not important at all. Just ignore it. Same with selecting the default font, that might not be available if the user removes it.