Add helper to load font

promag commented at 9:13 am on October 7, 2021: contributor

Originally submitted as https://github.com/bitcoin-core/gui-qml/pull/49.

promag renamed this:
~~qt: Add helper to load font~~
Add helper to load font
on Oct 7, 2021

hebasto commented at 11:57 am on October 7, 2021: member

Concept ACK.

in src/qt/guiutil.cpp:275 in 44f948fe14 outdated

271@@ -272,6 +272,12 @@ bool hasEntryData(const QAbstractItemView *view, int column, int role)
272     return !selection.at(0).data(role).toString().isEmpty();
273 }
274 
275+void loadFont(const QString& file_name)

hebasto commented at 12:00 pm on October 7, 2021:

Why not return id for generality?

From Qt docs:

An ID is returned that can be used to remove the font again with removeApplicationFont() or to retrieve the list of family names contained in the font.

promag commented at 12:11 pm on October 7, 2021:

The helper is meant for our use case: load font; ensure it is valid; remains loaded while the application runs.

Happy to add the return value, but at the moment it is discarded at the call site.

in src/qt/guiutil.cpp:277 in 44f948fe14 outdated

271@@ -272,6 +272,12 @@ bool hasEntryData(const QAbstractItemView *view, int column, int role)
272     return !selection.at(0).data(role).toString().isEmpty();
273 }
274 
275+void loadFont(const QString& file_name)
276+{
277+    int id = QFontDatabase::addApplicationFont(file_name);

hebasto commented at 12:01 pm on October 7, 2021:

0    const int id = QFontDatabase::addApplicationFont(file_name);

?

in src/qt/bitcoin.cpp:494 in 44f948fe14 outdated

490@@ -492,7 +491,7 @@ int GuiMain(int argc, char* argv[])
491 #endif
492 
493     BitcoinApplication app;
494-    QFontDatabase::addApplicationFont(":/fonts/monospace");
495+    GUIUtil::loadFont(":/fonts/monospace");

hebasto commented at 12:07 pm on October 7, 2021:

0    GUIUtil::loadFont(QStringLiteral(":/fonts/monospace"));

hebasto commented at 12:08 pm on October 7, 2021: member

Approach ACK 44f948fe142307810362f583bd8c230af428d78c.

hebasto added the label Refactoring on Oct 7, 2021

shaavan commented at 2:49 pm on October 7, 2021: contributor

Concept ACK

This PR introduces a new loadFont function, which along with adding the font, also asserts that it has been properly added to the QFontDatabase. I agree with the changes in the PR because this function eliminates any chance of font not being correctly added to the Application.

promag force-pushed on Oct 7, 2021

in src/qt/guiutil.h:117 in 98f423d39b outdated

112@@ -113,6 +113,11 @@ namespace GUIUtil
113 
114     void setClipboard(const QString& str);
115 
116+    /**
117+     * Loads the font from the file specified by fileName, aborts if it fails.

hebasto commented at 3:56 pm on October 7, 2021:

0     * Loads the font from the file specified by file_name, aborts if it fails.

in src/qt/guiutil.h:119 in 98f423d39b outdated

112@@ -113,6 +113,11 @@ namespace GUIUtil
113 
114     void setClipboard(const QString& str);
115 
116+    /**
117+     * Loads the font from the file specified by fileName, aborts if it fails.
118+     */
119+    void loadFont(const QString& file_name);

hebasto commented at 3:58 pm on October 7, 2021:

Capitalize the function name according to our Developer Notes?

hebasto approved

hebasto commented at 3:58 pm on October 7, 2021: member

ACK 98f423d39b12455360416086d877f2fa1a4a0bc9, tested on Linux Mint 20.2 (Qt 5.12.8).

qt: Add helper to load font d54ec27bac

promag force-pushed on Oct 7, 2021

hebasto approved

hebasto commented at 4:05 pm on October 7, 2021: member

re-ACK d54ec27bac388d7b84cf7b6cb4506bb0c25f2f88

stratospher commented at 4:46 pm on October 7, 2021: contributor

Tested ACK d54ec27. Refactoring the code and defining loadFont() in src/qt/guiutil.cpp reduces redundant imports of the QFontDatabase and is a better design.

Fonts in the application remain consistent in both the PR and master.

Master	PR

shaavan approved

shaavan commented at 12:08 pm on October 8, 2021: contributor

ACK d54ec27bac388d7b84cf7b6cb4506bb0c25f2f88

Changes since my last review:

The added function has been renamed from loadFont -> LoadFont according to the name convention
QStringLiteral macro is now used to ensure the input string is allocated statically.
const keyword is used for the id variable to indicate that it is not modified.
The fileName has been corrected to file_name in the comment.

hebasto merged this on Oct 9, 2021

hebasto closed this on Oct 9, 2021

promag deleted the branch on Oct 9, 2021

sidhujag referenced this in commit 62c9ed3f17 on Oct 9, 2021

luke-jr commented at 3:16 am on October 11, 2021: member

Why do we want to abort if it fails??

bitcoin-core locked this on Oct 11, 2022

Add helper to load font #448