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

Proposed by Evan McIntire
Status: Merged
Approved by: Stefano Verzegnassi
Approved revision: 167
Merged at revision: 184
Proposed branch: lp:~mcintire-evan/ubuntu-terminal-app/window-font-size
Merge into: lp:~ubuntu-terminal-dev/ubuntu-terminal-app/reboot
Diff against target: 24 lines (+5/-3)
1 file modified
src/app/qml/ubuntu-terminal-app.qml (+5/-3)
To merge this branch: bzr merge lp:~mcintire-evan/ubuntu-terminal-app/window-font-size
Reviewer Review Type Date Requested Status
Stefano Verzegnassi Approve
Jenkins Bot continuous-integration Approve
Review via email: mp+285285@code.launchpad.net

Commit message

Window size scales with font size

Description of the change

Window size scales with font size

To post a comment you must log in.
Revision history for this message
Evan McIntire (mcintire-evan) wrote :

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

Revision history for this message
Evan McIntire (mcintire-evan) wrote :

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

Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

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
Revision history for this message
Evan McIntire (mcintire-evan) wrote :

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!

Revision history for this message
Evan McIntire (mcintire-evan) wrote :

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

Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

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.

Revision history for this message
Evan McIntire (mcintire-evan) wrote :

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

Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

I replied to your question. :)

Revision history for this message
Evan McIntire (mcintire-evan) wrote :

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

Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

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

Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

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.

Revision history for this message
Evan McIntire (mcintire-evan) wrote :

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

167. By Evan McIntire

Actually scale with font size

Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/app/qml/ubuntu-terminal-app.qml'
2--- src/app/qml/ubuntu-terminal-app.qml 2016-02-07 18:24:56 +0000
3+++ src/app/qml/ubuntu-terminal-app.qml 2016-03-03 22:52:31 +0000
4@@ -12,9 +12,6 @@
5 applicationName: "com.ubuntu.terminal"
6 automaticOrientation: true
7
8- width: units.gu(90)
9- height: units.gu(55)
10-
11 AuthenticationService {
12 onDenied: Qt.quit();
13 }
14@@ -131,5 +128,10 @@
15
16 Component.onCompleted: {
17 tabsModel.selectTab(0);
18+
19+ // The margins for the terminal canvas are 2px
20+ // Hardcoded value from TerminalDisplay.h
21+ width = 80 * terminalPage.terminal.fontMetrics.width + 2
22+ height = 24 * terminalPage.terminal.fontMetrics.height + 2
23 }
24 }

Subscribers

People subscribed via source and target branches