nuttyartist / notes

Fast and beautiful note-taking app written in C++. Write down your thoughts.
https://www.get-notes.com
Mozilla Public License 2.0
3.6k stars 316 forks source link

Fixed loading of custom font icons #636

Closed zjeffer closed 10 months ago

zjeffer commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

guihkx commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

zjeffer commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

Alright lets see if this works

nuttyartist commented 10 months ago

Did it solve the problem?

zjeffer commented 10 months ago

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 commented 10 months ago

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

nuttyartist commented 10 months ago

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