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

Fixed loading of custom font icons #636

Merged
merged 1 commit into from
Aug 30, 2023
Merged

Conversation

zjeffer
Copy link
Collaborator

@zjeffer zjeffer commented Aug 15, 2023

Fixes #620

Apparently the QFont constructor doesn't load fonts from the application's database. We have to use QFontDatabase::font, which does load the fonts properly when otf-font-awesome is installed.

I replaced every occurence of QFont when loading our custom fonts, with the QFontDatabase::font constructor.

@zjeffer
Copy link
Collaborator Author

zjeffer commented Aug 15, 2023

Looks like the QML fonts are not yet fixed (top right):

image

image

I don't know much about QML, would you know if there's a similar way of loading the application database's fonts instead of the system's @nuttyartist ?

@nuttyartist
Copy link
Owner

Thanks for this!

I don't know much about QML, would you know if there's a similar way of loading the application database's fonts instead of the system's @nuttyartist ?

I'm quite sure the qml loads the app fonts and not the system fonts since the path to the fonts is qrc:/fonts/... which are embedded to the binary. So I'm not sure what's happening.

@nuttyartist
Copy link
Owner

I guess it won't hurt to merge this even tho we still didn't figure why the QML fonts won't load?

@guihkx
Copy link
Collaborator

guihkx commented Aug 23, 2023

I guess it won't hurt to merge this even tho we still didn't figure why the QML fonts won't load?

As long as all build jobs pass, I'm fine with that.

@zjeffer
Copy link
Collaborator Author

zjeffer commented Aug 23, 2023

Seems like it doesn't work for qt5 at the moment but I don't really have the time to figure out why at the moment.

@nuttyartist
Copy link
Owner

Seems like it doesn't work for qt5 at the moment but I don't really have the time to figure out why at the moment.

I'll look into it.

@nuttyartist
Copy link
Owner

nuttyartist commented Aug 23, 2023

The reason (per doc and error) is that in Qt 6 QFontDatabase::font is a static function and in Qt 5 it's not (so we need to istantiate an object for it).

Screen Shot 2023-08-23 at 7 22 59 PM

Screen Shot 2023-08-23 at 7 24 03 PM

Should we just create an object for both Qt 6 and 5 or do everything with #if?

@nuttyartist
Copy link
Owner

nuttyartist commented Aug 24, 2023

I actually don't know why calling QFontDatabase::font should be needed, the docs don't seem to say that this is any different than loading via QFont's constructor. But I did notice we need to load two more fonts in main.cpp

if (QFontDatabase::addApplicationFont(":/fonts/fontawesome/fa-regular-400.ttf") < 0)
        qWarning() << "FontAwesome Regular cannot be loaded !";

    if (QFontDatabase::addApplicationFont(":/fonts/fontawesome/fa-brands-400.ttf") < 0)
        qWarning() << "FontAwesome Brands cannot be loaded !";        

Tho I don't think these are the fonts that don't load for you.

@nuttyartist
Copy link
Owner

I do see now that there are both QFontDatabase::font and QFontDatabase::systemFont so you're probably right.

@zjeffer
Copy link
Collaborator Author

zjeffer commented Aug 27, 2023

Should we just create an object for both Qt 6 and 5 or do everything with #if?

I think maybe creating a separate class that loads fonts might be the best way. In that class, we can use a single #if. I'll look into trying that.

@nuttyartist
Copy link
Owner

Should we just create an object for both Qt 6 and 5 or do everything with #if?

I think maybe creating a separate class that loads fonts might be the best way. In that class, we can use a single #if. I'll look into trying that.

Ahaha sounds promising.

@zjeffer
Copy link
Collaborator Author

zjeffer commented Aug 27, 2023

Alright lets see if this works

@nuttyartist
Copy link
Owner

Did it solve the problem?

@zjeffer
Copy link
Collaborator Author

zjeffer commented Aug 28, 2023

All checks seem to have passed and it seems to work on my system (linux, qt6), but we need to test it on other platforms as well. Can someone test on Windows & macOS?

@nuttyartist
Copy link
Owner

I tested on macOS and seems to be fine. I can test Windows tonight/tomorrow morning.

Copy link
Collaborator

@guihkx guihkx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine on Ubuntu 20.04 / Qt 5.12.

@nuttyartist
Copy link
Owner

Tested on Windows, working well. Thanks, @zjeffer! Merging...

@nuttyartist nuttyartist merged commit 7466cfb into master Aug 30, 2023
17 checks passed
@guihkx guihkx deleted the fix/unicode-fonts-620 branch August 30, 2023 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Icons not showing properly
3 participants