Merge lp:~nikwen/ubuntu-terminal-app/font-size-fix into lp:~ubuntu-terminal-dev/ubuntu-terminal-app/reboot

[Niklas Wenzel]

Make font size relative to "medium" fontSize for Labels

Default font size is set to be the same as that of gnome-terminal on a Unity 7 desktop

Make font size relative to "medium" fontSize for Labels

Default font size is set to be the same as that of gnome-terminal on a Unity 7 desktop

[Niklas Wenzel (nikwen)]

While working on this I found https://bugs.launchpad.net/ubuntu-terminal-app/+bug/1542668.

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

PASSED: Continuous integration, rev:163
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/1004/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/554/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/1004/rebuild

review: Approve (continuous-integration)

[Stefano Verzegnassi (verzegnassi-stefano)]

The MP looks good, although I'm not sure on the default and the maximum size for the font.
I've left two inline comments.

Also, it might be nice to handle the font size in terms of 100th in the settings page (i.e. from 4% to 500% - assuming the current range is ok).

review: Needs Information

[Niklas Wenzel (nikwen)]

Thank you for the review! :)

Here is a comparison of the font sizes with the current default: https://launchpadlibrarian.net/236776246/comparison.png

This shows that the values from the documentation are probably incorrect. However, you're right that it looks better on the phone with a ration of 1.0. In other words: perfect. So 10 might be better as the default.

For the range, have a look at this screenshot with maximum size: https://launchpadlibrarian.net/236776437/max-size.png
That's probably due to the incorrect desktop scaling again, though. On the phone, 50 is clearly too big.

We have to keep in mind to also consider cases where people want to present something using a projector. That's why we need a maximum that isn't too low.

[Niklas Wenzel (nikwen)]

I have to say that I am against the 100th, though. In general I tend to change appearance settings in applications once and afterwards often want to go back to the default. This is doable with the current 10th but it would be a hassle to hit exactly 1.0 when we use 100.
Furthermore, I guess that the activity of changing the font size is much more fulfilling when you do not have to ask yourself if it would look better with 1% more or less (assuming you're a perfectionist).

That's why I would vote for the current range but changing the default to 1.0 as the majority of our users are currently phone users.

[Stefano Verzegnassi (verzegnassi-stefano)]

>Here is a comparison of the font sizes with the current default: >https://launchpadlibrarian.net/236776246/comparison.png
>
>This shows that the values from the documentation are probably incorrect. However, you're right that it looks better on the phone with a ration of 1.0. In other words: perfect. So 10 might be better as the default.

Strange, I gave a "console.log(FontUtils.sizeToPixels("medium"))" on my PC and returns 14px. :/
Anyway, as long as we both believe that using 1.0x factor returns a better result on phones, we don't need to discuss about it :D

>We have to keep in mind to also consider cases where people want to present something using a projector. That's why we need a maximum that isn't too low.

Good point! Since on my 22" screen I was able to fit 40 chars, it's still a good value for the scenario you described.

>I have to say that I am against the 100th, though. In general I tend to change appearance settings in applications once and afterwards often want to go back to the default. This is doable with the current 10th but it would be a hassle to hit exactly 1.0 when we use 100.

Legit! I was thinking about a smaller range [50%, 250%] and percentage would have probably worked better.
Since we'll stay with the proposed range, it's okay to me not to change this (although I feel it a bit strange).

Let's approve it then!

review: Approve

[lp:~nikwen/ubuntu-terminal-app/font-size-fix]

164. By Niklas Wenzel on 2016-02-06

Change default font size to 10

165. By Niklas Wenzel on 2016-02-06

Revert po/ folder to fix merge conflict

[Niklas Wenzel (nikwen)]

Thank you for the quick review again! :)

I changed the default back to 10 and tested it on my phone again. I'll top-level approve this now to get it merged.

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

PASSED: Continuous integration, rev:165
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/1008/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/559/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/1008/rebuild

review: Approve (continuous-integration)

=== modified file 'src/app/qml/TerminalComponent.qml'
--- src/app/qml/TerminalComponent.qml	2016-01-09 01:47:56 +0000
+++ src/app/qml/TerminalComponent.qml	2016-02-06 18:10:29 +0000
@@ -12,7 +12,7 @@
 
         colorScheme: settings.colorScheme
         font.family: settings.fontStyle
-        font.pointSize: settings.fontSize
+        font.pixelSize: FontUtils.sizeToPixels("medium") * settings.fontSize / 10
 
         signal sessionFinished(var session);
 

=== modified file 'src/app/qml/TerminalSettings.qml'
--- src/app/qml/TerminalSettings.qml	2015-12-19 05:14:39 +0000
+++ src/app/qml/TerminalSettings.qml	2016-02-06 18:10:29 +0000
@@ -11,9 +11,9 @@
     property alias showKeyboardBar: innerSettings.showKeyboardBar
     property alias showKeyboardButton: innerSettings.showKeyboardButton
 
-    readonly property int defaultFontSize: units.gu(0.4)
-    readonly property int minFontSize: 2
-    readonly property int maxFontSize: 32
+    readonly property int defaultFontSize: 10
+    readonly property int minFontSize: 4
+    readonly property int maxFontSize: 50
 
     property alias jsonVisibleProfiles: innerSettings.jsonVisibleProfiles