Review: Needs Fixing
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?
Diff comments:
> === modified file 'src/app/main.cpp'
> --- src/app/main.cpp 2016-01-25 12:37:14 +0000
> +++ src/app/main.cpp 2016-02-13 21:25:20 +0000
> @@ -53,7 +53,7 @@
> {
> QApplication a(argc, argv);
> QQuickView view;
> - view.setResizeMode(QQuickView::SizeRootObjectToView);
> + view.setResizeMode(QQuickView::SizeViewToRootObject);
Changing the resize mode is not a good idea, because of the way users interact
with the window :)
1) Try to maximize the window with a font size = 10.
Result: Main QML item is not resized with the window.
2) Try to maximize the window with a very large font (e.g. 34)
Result: you can't access to the buttons that are usually displayed at the right
of the window.
See screenshot: https://imgur.com/e6Gqjhi
3) If you change the size of the window by dragging its border (common
operation on desktop), the QML object is not resized.
So please restore the old QQuickView::SizeRootObjectToView value.
========
Quote from your comment:
> 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
Reverting to QQuickView::SizeRootObjectToView does not fix this.
But, for example, Konsole does not resize the window after a font size
adjustment. :)
If you want to emulate the behaviour of gnome-terminal (given you're using
QQuickView::SizeRootObjectToView), I'd suggest you to expose QQuickView's
properties to QML.
Add in main.cpp:
// Expose QQuickView properties for window resize handling
view.engine()->rootContext()->setContextProperty("View", &view);
In TerminalPage.qml define the "terminal" property as QMLTermWidget type.
Then, in the MainView you can add something like:
Connections {
target: terminalPage.terminal
onChangedFontMetricSignal: {
View.width = mview.width
View.height = mview.height
}
}
So you can update the window size according the font size adjustment, only when
the font has been adjusted (i.e. you avoid unpredictable bindings).
Anyway, we still don't get the same behaviour, since gnome-terminal always keep
the same number of line/columns when the font size has changed.
e.g.
1) Open gnome-terminal. Default size is 80x24 (chars)
2) Manually resize the window. Let's say 100x24 (chars)
3) Increase font size. The window is still 100x24 (chars).
I wouldn't try to mimic gnome-terminal on this, since it would require some
extra logic which is IMHO not good-looking on QML.
>
> FileIO fileIO;
> view.engine()->rootContext()->setContextProperty("fileIO", &fileIO);
>
> === 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-02-13 21:25:20 +0000
> @@ -12,8 +12,8 @@
> applicationName: "com.ubuntu.terminal"
> automaticOrientation: true
>
> - width: units.gu(90)
> - height: units.gu(55)
> + width: 40 * settings.fontPixelSize()
You sholudn't use the size specified in the settings, but you should get it
from the terminal widget instead (of course, IMHO).
You've hardcoded the 1:2 ratio of the Ubuntu Mono font but not all Mono fonts
use that ratio.
> + height: 24 * settings.fontPixelSize()
>
> AuthenticationService {
> onDenied: Qt.quit();
--
https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/window-font-size/+merge/285285
Your team Ubuntu Terminal Developers is subscribed to branch
lp:ubuntu-terminal-app.
--
Mailing list: https://launchpad.net/~ubuntu-touch-coreapps-reviewers
Post to : ubuntu-touch-coreapps-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~ubuntu-touch-coreapps-reviewers
More help : https://help.launchpad.net/ListHelp
