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

Revision history for this message
Kill Animals (kill-animals) wrote :

> Agreed, since those are set only on construction we should simply put everything in Component.onComplete (where they should work in theory).

Sounds good.

> We still need to remove the overloaded setVTFont(font, size) since I think that's unnecessary code duplicatoin and might be replaced by something like that in Component.onCompleted:

Component.onCompleted: {
    kterm.font.pointSize: jsConf.getFontSize()
    kterm.font.family: jsConf.getFontStyle()
    //Something else
}

I'll have to look at it. Remember; I overloaded the function on purpose because I needed a way to set the font on the fly, and to set the font, I need QFont, which requires size and family parameters. Component.onCompleted afaik will only set it initially on startup. Not when I go into settings and adjust it.
Either way, if I can eliminate the overload, i'll do it.

> This is surely a problem with how we are initializing the plugin. It is something that might be worth checking in the future, but if we can force them from Component.onCompleted the pedantic me would be happy enough :)

Yah I guess it does need to be fixed. Was font.xxxx defined in this program? If that is the case, then I can't imagine it being too difficult. Either way; worst case scenario leaves us with this small patch.

In the spirit of keeping merges small, can we patch the "Advanced Options" in a future patch?

« Back to merge proposal