ubuntu-touch-coreapps-reviewers team mailing list archive

[Stefano Verzegnassi <stefano92.100@xxxxxxxxx>]

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.