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

> 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.

No no; Intrude !

> 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).

We could put an "Advanced Options" selection at the bottom; what do you think about that?

> 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?

Uhm, I copied the mechanism that was being used for setting the font size, which was done by someone else. Q_INVOKABLE (Although I initially researched it, and wondered why it was not being utilized for this) is not used for the font. I do not know why, but needless to say; it is not in my merge at all. Could you please clarify your comments? Perhaps point to the code you are speaking of?

> 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.

Oh I see what you mean. I do not think that actually will cause anything to break though. The font is not being read as an index; the font is being read as a string. The font is being written as an index, and that write is only triggered on an index change.

ergo

The user will have to physically select the font to something else.
When he physically selects the font, the index will be corrected at that point, updating to the current list.
After the index is changed, the model[index] returns the font selected to the database.
This triggers the configChanged
The configChanged triggers a read to the database, which returns the appropriate string.