High chance of font family conflicts with bundled fonts

kumattau commented 2 years ago

Hi,

For some fonts that qtawesome has, family remains the default. This may break the user application or qtawesome if the user adds a different version of the font.

For example, in the following code, if the different version of "Material Design Icon" by QtGui.QFontDatabase.addApplicationFont("materialdesignicons-webfont.ttf") is added, qtawesome icon disappers.

QtGui.QFontDatabase.addApplicationFont affects application-wide, so qtawesome's internal "Materinal Design Icon" seems to be overwritten by user's one.

I think it's a good idea to add a unique string (e.g "(qtawesome)") to all families of qtawesome's internal icons to avoid the confliction.


# -*- coding: utf-8 -*-
import sys
from qtpy import QtCore, QtWidgets, QtGui
import qtawesome as qta

class AwesomeExample(QtWidgets.QDialog):

    def __init__(self):
        super().__init__()

        # Get Material Design icons by name
        mdi6_icon = qta.icon('mdi6.account-wrench')
        mdi6_button = QtWidgets.QPushButton(mdi6_icon, 'Material Design Icon # Version 6.3.95, ch=F189A')

        #
        # Add different version of 'Material Design Icon' from qtawesome's internal
        # For example, if version 6.2.95 is added, mdi6.account-wrench (ch=F1809A) disappers
        #
        QtGui.QFontDatabase.addApplicationFont("materialdesignicons-webfont.ttf")

        font = QtGui.QFont("Material Design Icon")
        fontMetrics = QtGui.QFontMetrics(font)
        print(f"font.key()                      : {font.key()}")
        print(f"fontMetrics.inFontUcs4(0xF189A) : {fontMetrics.inFontUcs4(0xF189A)}")

        grid = QtWidgets.QGridLayout()
        grid.addWidget(mdi6_button, 0, 0)
        self.setLayout(grid)
        self.setWindowTitle('Awesome')
        self.show()

def main():
    app = QtWidgets.QApplication(sys.argv)
    if hasattr(QtCore.Qt, 'AA_UseHighDpiPixmaps'):
        app.setAttribute(QtCore.Qt.AA_UseHighDpiPixmaps)
    QtCore.QTimer.singleShot(10000, app.exit)
    _ = AwesomeExample()
    sys.exit(app.exec_())

if __name__ == '__main__':
    main()

QtGui.QFontDatabase.addApplicationFont("materialdesignicons-webfont.ttf") is not executed
QtGui.QFontDatabase.addApplicationFont("materialdesignicons-webfont.ttf") is executed

dalthviz commented 2 years ago

Hi @kumattau thanks for the feedback! Makes sense to me to somehow differentiate by some sort of prefix the font names that are being bundled here to help reduce the possibility of collisions when adding custom fonts :+1:

Just in case, what do you think @ccordoba12 ?

ccordoba12 commented 2 years ago

Yeah, I think that's a good idea. However, I don't know if simply renaming the fonts could do that, or if there's something that's necessary.

kumattau commented 2 years ago

Thanks @ccordoba12,

Yeah, I think that's a good idea. However, I don't know if simply renaming the fonts could do that, or if there's something that's necessary.

I think changing family is enough to resolve the collision.

https://doc.qt.io/qt-5/qfont.html says the font is matched as follows:

The font matching algorithm works as follows:

The specified font families (set by setFamilies()) are searched for.

If not found, then if set the specified font family exists and can be used to represent the writing system in use, it will be selected.

If not, a replacement font that supports the writing system is selected. The font matching algorithm will try to find the best match for all the properties set in the QFont. How this is done varies from platform to platform.

If no font exists on the system that can support the text, then special "missing character" boxes will be shown in its place.

I think, by above acondition 1, changing family of the qtawesome's font, user's choice does not match qtawesome's font and qtawesome's choice does not match user's font, so the collision does not occur.

Also, changing family was used to resolve the following qtawesome internal collision and seems to be working fine.

the collision of multiple awesome fonts : #107, #97
the collision of multiple material design icon fonts : #174 3d8bdca298f6e284d45288728fe46156a1adb738

ccordoba12 commented 2 years ago

I think changing family is enough to resolve the collision.

Ok, great! Could you give us a hand with that?

kumattau commented 2 years ago

Ok, great! Could you give us a hand with that?

Yes, I will try it soon.

kumattau commented 2 years ago

It is better that updating font and changing family can be processed with update script, not manually. But I feel current directory structure makes difficult to process it automatically.

For example, 5.x and 6.x material design icons are bundled in fonts. These icons were distributed with same filename, so I renamed the filename manually.

To realize updating font and changing family, I think it it better to change the directory structures as follows.

./fonts/mdi/materialdesignicons-webfont.ttf
  keep filename as destributed
  material design icons 5.9.55
  family: qta+174c02@Material Design Icons
./fonts/mdi/materialdesignicons-webfont-charset.json
  charset of material design icons 5.9.55

./fonts/mdi6/materialdesignicons-webfont.ttf
  keep filename as destributed
  material design icons 6.3.55
  family: qta+9a2f45@Material Design Icons
./fonts/mdi6/materialdesignicons-webfont-charset.json
  charset of material design icons 5.3.55

other fonts are the same.

"qta+9a2f45@" is prefix to avoid the collision. "9a2f45" is md5sum of original ttf file.

Additionally, the all charset is seems to be destributed in the form of css file, so I think it is better to can parse css file directly. https://pypi.org/project/cssutils may be useful for the purpose.

Above changes aren't affect public API but it is much big. I trying to implement but Is the try worth it ? I am worried that big change cannot be accepted for the project.

dalthviz commented 2 years ago

Thanks @kumattau for checking! I would say that adding the prefix when using the updating script automatically would be nice. Just in case you think the current folder structure makes it difficult because you will need to use a different font file name? I think that is done by the update script for material design 6.x when downloading the font file (materialdesignicons6-webfont.ttf), right?

Anyhow not totally sure of the need of changing the folder structure, but if you think that will make the font updates easier I guess is ok. Just in case, what do you think @ccordoba12 ?

Regarding the CSS file parsing not sure if needed with an external dependency (my guess is that the way is currently working is to avoid using an external dependency). Is that causing also conflicts somehow? If not maybe that is something to review in another issue.

Thanks again for sharing what you were thinking for the implementation!

ccordoba12 commented 2 years ago

Just in case, what do you think @ccordoba12 ?

I think it's fine as long as updating to new font versions remains easy.

kumattau commented 2 years ago

Thank you @dalthviz , @ccordoba12.

Just in case you think the current folder structure makes it difficult because you will need to use a different font file name?

Yes, material design icon has same filename between 5.x and 6.x. so I need to rename them to avoid filename collision.

I think that is done by the update script for material design 6.x when downloading the font file (materialdesignicons6-webfont.ttf), right?

Yes. if update script knows that materialdesignicons-webfont.ttf should be saved as materialdesignicons6-webfont.ttf, the collision is avoided. The advantage of using sub directories is that the update script does not need to know the name of the file to be saved.

Here's a way I came up with to avoid file name collisions.

move font and charmap to the corresponding "fonts/<prefix>"` sub directory
rename font and charmap to "qta+<md5>@<original filename>"

Of these, I thought the sub-directory method was easier to understand because the original file name can be kept and there is no need to define it in the script. As for the rename method, I think it is difficult to manage the differences because the file name changes with each update. The method of defining the file name in script or in the configuration file may be fine for managing bundled fonts.

Regarding the CSS file parsing not sure if needed with an external dependency (my guess is that the way is currently working is to avoid using an external dependency). Is that causing also conflicts somehow? If not maybe that is something to review in another issue.

Currently, regular expression is used for the conversion from css to json. The regular expression is working fine, but if the format of upstream css is changed, it is not sure to keep to work. I think css parser is better than the regular expression, but is no current problem because the regular expression is working now.

I'll look into how to implement it more.

kumattau commented 2 years ago

I've tried prototyping it, but the process of updating each font (downloading, adding prefix to family, updating checksum, etc.) is duplicated, so I think I should also refactor setupbase.py.

It may take some time, but I will send a PR when I have a draft.

kumattau commented 2 years ago

Can I use dictionary ordering support or dataclass which requires python 3.7 or later to implement font updater simply?

dalthviz commented 2 years ago

I think it should be fine :+1:

I think we should drop support for Python 3.6 in the future (probably before merging the improvements you are working on).

Thanks for asking @kumattau !

spyder-ide / qtawesome

High chance of font family conflicts with bundled fonts #188