PR: Return original font directory when font user installation load fails

[dalthviz]

Fixes #264

[ccordoba12]

Instead of returning fonts_directory below, why don't we raise a FontError there, saying that it was not possible to load a specific font? The error message could also use get_fonts_info to tell users what fonts they need to install manually and where they are located.

[dalthviz]

After giving it some thought, if we raise FontError here the fallback logic to use system wide fonts will never be reached (since then always an error here will be raised). Maybe we should show instead a warning here and only in case we are unable to detect the font that failed being loaded is available from the system fonts directory? What do you think @ccordoba12 ?

[dalthviz]

Last commit implements the idea above but let me know what you think

[ccordoba12]

Ok, that looks better to me. But then I think we don't need to return fonts_directory below, but a simple return would work as well. And the comment you added here should be reworded, right?

[dalthviz]

Checking, we need to return fonts_directory, otherwise you will end up with a TypeError around the code that loads the fonts at

qtawesome/qtawesome/iconic_font.py

Lines 404 to 416 in 507aa4e

	if directory is None:
	directory = self._get_fonts_directory()
	# Load font
	if QApplication.instance() is not None:
	with open(os.path.join(directory, ttf_filename), "rb") as font_data:
	data = font_data.read()
	id_ = (
	-1
	if os.environ.get("QTA_FORCE_SYSTEM_FONTS_LOAD") and os.name == "nt"
	else QFontDatabase.addApplicationFontFromData(data)
	)
	font_data.close()

This error would raise since _get_fonts_directory would end up returning None due to _get_fonts_directory currently being in charge of calling the install logic and returning the value given

qtawesome/qtawesome/iconic_font.py

Lines 655 to 668 in 507aa4e

	def _get_fonts_directory(self):
	"""
	Get bundled fonts directory.
	On Windows an attempt to install the fonts per user is done
	to prevent errors with fonts loading.
	See spyder-ide/qtawesome#167 and spyder-ide/spyder#18642 for
	context.
	"""
	fonts_directory = os.path.join(
	os.path.dirname(os.path.realpath(__file__)), "fonts"
	)
	return self._install_fonts(fonts_directory)

Thinking about this, if we would want to be able to return None over the _install_fonts method, maybe we should prevent _get_fonts_directory from returning None? So maybe _get_fonts_directory should look something like:

        fonts_directory = os.path.join(
            os.path.dirname(os.path.realpath(__file__)), "fonts"
        )
        install_fonts_directory = self._install_fonts(fonts_directory)
        return install_fonts_directory if install_fonts_directory else fonts_directory 

?

Let me know what you think @ccordoba12

[ccordoba12]

No, I think it's fine to leave things as they are. I just noticed that _install_fonts returns fonts_directory in several places. So, let's not change that logic in this PR because it's not needed to fix issue #264.

I'll leave a suggestion to improve this comment, then this should be ready.

[ccordoba12]

Sorry for closing this PR (I don't know why it happened).

[ccordoba12]

Some improvements to the comment and warning text, then this should be ready.

[ccordoba12]

Looks good to me now, thanks @dalthviz!