Code review comment for lp:~kill-animals/ubuntu-terminal-app/1349749workaround-select-font-added-to-preferences

Revision history for this message
Filippo Scognamiglio (flscogna) wrote :

Sorry for the intrusion but I'd like to share my two cents on this merge proposal.

First of all I agree with David that we should not overwhelm the users with settings but I also think that font selection might be worthy (on the desktop they are mandatory on the phone maybe not).

Anyway I really think the implementation might use some work. Before we were using a Q_PROPERTY to set the font. Why are you making setVTFont Q_INVOKABLE and calling it directly instead of using the previous mechanism?

Moreover storing the selected font as an index is not wise when we take into account future updates since we will be forced to add new fonts only at the end of the list. I suggest to store the font.family string in the db and then compute the index.

review: Needs Fixing

« Back to merge proposal