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

Sorry for the previous fast comment, but I wanted to write that before launch! :D

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

Yeah, that might be the sweet spot; Anyway might be interesting to have the opinion of a UX expert (I'm definitely not).

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

Q_INVOKABLE are functions that can be called from the QtQuick side. In this case you are directly calling setVTFont(const QString& s, int n) from QML:

Terminal.qml:236 font.family: kterm.setVTFont(jsConf.getFontStyle(), kterm.font.pointSize);

What you are doing it is calling the invokable function setVTFont(font, size), grab the return type (void) and assign that to the property font.family. That might even work but it is extremely convoluted.
The solution I propose is to get rid of the overloaded setVTFont(font, size) and using property assignments as you are doing here.

Terminal.qml:88 kterm.font.pointSize = jsConf.getFontSize();
Terminal.qml:89 kterm.colorScheme = jsConf.getColorScheme();
Terminal.qml:90 kterm.font.family = jsConf.getFontStyle();

The property will then trigger the setVTFont(font) function, preserving the plugin interface.
I really hope that's clear enough (I still have issues explaining non trivial concepts in english).

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

Forget that, I'm an idiot. You was suggesting you to do exactly what you did! Sorry :)

I just noticed the changes to TerminalDisply.h... Awesome! :D