Merge lp:~mcintire-evan/ubuntu-terminal-app/window-font-size into lp:~ubuntu-terminal-dev/ubuntu-terminal-app/reboot

[Evan McIntire]

Window size scales with font size

Window size scales with font size

[Evan McIntire (mcintire-evan)]

This was based in part of Nikwen's work in here[1], I still have a bit to do before I can call this finished, though

I think the " FontUtils.sizeToPixels("medium") * settings.fontSize / 10 " should be moved into a function so if any changes need to be made to it, we don't have to hunt down all the places it is used, but I'm not sure where I should place such a function

Also, the window itself does not scale with the MainView if the font size is changed while running, so I need to do a bit of work to address that

[1] - https://bazaar.launchpad.net/~ubuntu-terminal-dev/ubuntu-terminal-app/reboot/revision/165

[Evan McIntire (mcintire-evan)]

https://i.imgur.com/01kJUIB.png Screen of this overlayed with gnome-term with background transparency

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

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

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

review: Approve (continuous-integration)

[Stefano Verzegnassi (verzegnassi-stefano)]

I left two inline comments.

As a summary, here's a diff with the changes I've proposed (line 73 is wrong - that "imports" is not required).

The big problem with this MP is the usage of QQuickView::SizeViewToRootObject.
In my opinion it creates a lot of problems (speaking in general terms) because it breaks the "Window > Contents" relationship which is at the base of a window system.

For that reason my review is a "Needs Fixing".

======

A few comments on my diff:

1) About the "Connections {}" in MainView:

It's not strictly required. I mean, in your comment above you said that you would like to reproduce that specific behaviour, but e.g. Konsole does not.

I've been a Kubuntu users in that past, and the GTK world and the KDE (Qt) world used to be very different on such things. That's why many GTK apps do things that the KDE counterpart do not, and vice versa.
Ubuntu, as a GTK distro which is moving to Qt/QML, stays in between.

So, if you want to adjust the window size after a font size change, I'd suggest you to keep the logic very simple as I do.

Things could be done better but, as long as we stay with QQuickView and don't move to QQmlApplicationEngine, that's the cleanest solution.

2) I've replaced your fontPixelSize() function with a read-only property. That's not really required, but I did it for prototyping.

3) That "onChangedFontMetricSignal" signal has a terrible name. Would you mind renaming it as "onFontMetricsChanged" in the QMLTermWidget?

review: Needs Fixing

[Evan McIntire (mcintire-evan)]

Thanks for the long reply, I appreciate it! I somehow didn't realize that resizing manually lead to issues, I guess I need to test this stuff a lot better before pushing it :P

What you say makes a ton of sense, so I'll be reverting this latest revision and cleaning up a bit, thanks again!

[Evan McIntire (mcintire-evan)]

Sorry for the mass delays on this, school has been keeping me quite busy :) I'll be able to get on this tomorrow (hopefully)

[Stefano Verzegnassi (verzegnassi-stefano)]

Hi Evan,
No problem, I've been busy with my exams too. :)

I've planned to spend some time on terminal-app tomorrow. Feel free to ping whenever you want on IRC/mail/mailing list/telegram if you need to ask something.

[Evan McIntire (mcintire-evan)]

Finally got some time to look at this, left some replies to your comments

[Stefano Verzegnassi (verzegnassi-stefano)]

I replied to your question. :)

[Evan McIntire (mcintire-evan)]

Added another reply - we're getting so close here!

[Stefano Verzegnassi (verzegnassi-stefano)]

And here we go! I've found the reason of such strange behaviour!

[Stefano Verzegnassi (verzegnassi-stefano)]

Huh, mark the comment about the 2px value as WORKAROUND, in case we'll decide in a distant future to move from the current code to a more "QML-friendly" implementation.

[Evan McIntire (mcintire-evan)]

You're a champ, Stefano. New revision in just a moment :)

[lp:~mcintire-evan/ubuntu-terminal-app/window-font-size]

167. By Evan McIntire on 2016-03-03

Actually scale with font size

[Stefano Verzegnassi (verzegnassi-stefano)]

I guess you decided to set the values from the 'Component.onCompleted' event in order to prevent the window size from being updated after a change of the font size.

That's okay to me, it's the Konsole behaviour we discussed earlier.

Looks good to me! Thanks Evan!

review: Approve

=== modified file 'src/app/qml/ubuntu-terminal-app.qml'
--- src/app/qml/ubuntu-terminal-app.qml	2016-02-07 18:24:56 +0000
+++ src/app/qml/ubuntu-terminal-app.qml	2016-03-03 22:52:31 +0000
@@ -12,9 +12,6 @@
     applicationName: "com.ubuntu.terminal"
     automaticOrientation: true
 
-    width: units.gu(90)
-    height: units.gu(55)
-
     AuthenticationService {
         onDenied: Qt.quit();
     }
@@ -131,5 +128,10 @@
 
     Component.onCompleted: {
         tabsModel.selectTab(0);
+
+        // The margins for the terminal canvas are 2px
+        // Hardcoded value from TerminalDisplay.h
+        width = 80 * terminalPage.terminal.fontMetrics.width + 2
+        height = 24 * terminalPage.terminal.fontMetrics.height + 2
     }
 }