Migrate QQC1 to QQC2
ClosedPublic

Authored by guoyunhe on Sun, Oct 20, 10:39 AM.

Details

Summary

The TableView in digital clock widget time zone configuration is replaced with ListView. Other UI didn't change.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
guoyunhe created this revision.Sun, Oct 20, 10:39 AM
Restricted Application added a project: Plasma. · View Herald TranscriptSun, Oct 20, 10:39 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
guoyunhe requested review of this revision.Sun, Oct 20, 10:39 AM
guoyunhe edited the summary of this revision. (Show Details)
ngraham requested changes to this revision.Mon, Oct 21, 4:01 AM
ngraham added a subscriber: ngraham.

++. The new table in the clock settings is much nicer anyway. :) See the following inline comments:

applets/digital-clock/package/contents/ui/configTimeZones.qml
21–22

import QtQuick.Controls 2.5 as QQC2

48

While you're doing some porting, this entire thing can be replaced with a Kirigami.InlineMessage which will drastically reduce the amount of code here, and you can also remove the PlasmaComponents import (PlasmaCore is still needed for units, unless you want to also port things to use Kirigami.Units instead)

70

units.largeSpacing is preferable to hardcoded spacing values

95

This is pretty hard to follow. I would recommend expanding it to a conventional if/else block for clarity.

applets/digital-clock/plugin/timezonesi18n.cpp
463 ↗(On Diff #68345)

Seems unrelated; probably best done in another patch

applets/systemmonitor/common/contents/ui/ConfigGeneral.qml
20

import QtQuick.Controls 2.5 as QQC2

126

Add editable: true and an appropriate valueFromText converter

sddm-theme/BreezeMenuStyle.qml
6 ↗(On Diff #68345)

import QtQuick.Controls 2.5 as QQC2

sddm-theme/KeyboardButton.qml
6–7

import QtQuick.Controls 2.5 as QQC2

sddm-theme/Main.qml
23

import QtQuick.Controls 2.5 as QQC2

sddm-theme/SessionButton.qml
25–26

import QtQuick.Controls 2.8 as QQC2

wallpapers/image/imagepackage/platformcontents/phone/ui/config.qml
25

import QtQuick.Controls 2.5 as QQC2

wallpapers/image/imagepackage/platformcontents/phone/ui/customwallpaper.qml
25

import QtQuick.Controls 2.5 as QQC2

This revision now requires changes to proceed.Mon, Oct 21, 4:01 AM
kmaterka added inline comments.
applets/digital-clock/package/contents/ui/configTimeZones.qml
48

You can take my changes from D24853

guoyunhe updated this revision to Diff 68733.EditedFri, Oct 25, 8:19 AM

Merge kmaterka's patch D24853

The inline message box looks nice!

That new time zone view lacks any form of keyboard navigation. I cannot use arrow keys to navigate it and neither can I hit space to toggle a checkbox. It also lacks a frame around its content area like the text field has.

guoyunhe updated this revision to Diff 69701.Wed, Nov 13, 8:08 PM

Add QQC2 namespace

Needs a rebase, and Kai's comment still needs to be addressed.

ngraham requested changes to this revision.Wed, Nov 13, 8:52 PM

Thanks. Still needs fixes for the timezone list:

  • No frame around list
  • Scrolling using touchpad or mouse wheel does not work
  • Scrollview bounds are too large
  • Item highlight is weird and nonstandard
  • Keyboard navigation/selection does not work

QQC2 really shouldn't make it this hard to build a halfway decent list view...

This revision now requires changes to proceed.Wed, Nov 13, 8:52 PM
guoyunhe updated this revision to Diff 69705.Wed, Nov 13, 8:56 PM

Prefix QQC2

guoyunhe added a comment.EditedThu, Nov 14, 8:21 AM

Maybe the border of ScrollView is missing in breeze theme. I checked some other widgets, like network manager:

It also miss the border in the connection list.

guoyunhe updated this revision to Diff 69730.EditedThu, Nov 14, 9:16 AM

Enable keyboard navigation and proper highlight

The key point is to enable focus properly:

  1. focus: true to both ListView and its delegate item. They are not focusable by default.
  2. activeFocusOnTab: true to ListView. So when you press Tab, it get focused.
  3. Call forceActiveFocus() function when clicking delegate item, so the ListView get focused.

Basically QML ListView doesn't response to any focus things by default. You have to enable them one by one. Then keyboard navigation will work.

Arrow key for navigation, Space for toggling checkbox. Enter will quit the window?

Maybe the border of ScrollView is missing in breeze theme. I checked some other widgets, like network manager:

It also miss the border in the connection list.

QQC2 Dekstop Style ScrollView does something really wonky with its background, where it only creates one if it was not set, instead of just having a background that can be overridden. In addition, that background is hidden by default. To enable it, you will need to add Component.onCompleted: background.visible = true to your scrollview.

guoyunhe updated this revision to Diff 69739.Thu, Nov 14, 12:32 PM

Thanks for the tip. It seems added the border, but when scrolling, the top border or bottom border disappear. It cannot show full border at all sides.

GB_2 added a subscriber: GB_2.Thu, Nov 14, 12:45 PM

Thanks for the tip. It seems added the border, but when scrolling, the top border or bottom border disappear. It cannot show full border at all sides.

Yeah, that is a general pre-existing bug.

I also tried to not use ScrollView but enable ListView's scrollbar:

ListView {

    // add scrollbar, background and border
    clip: true
    flickableDirection: Flickable.VerticalFlick
    boundsBehavior: Flickable.StopAtBounds
    QQC2.ScrollBar.vertical: QQC2.ScrollBar { }
}

But it has no border or background. And the scrolling speed is crazily fast when using touch pad... However, all Qt examples on the internet say it is the way to add scrollbar to Flickables. But it is really buggy. https://bugreports.qt.io/browse/QTBUG-56075

ngraham accepted this revision.Thu, Nov 14, 5:20 PM

LGTM! Very nice work.

This revision is now accepted and ready to land.Thu, Nov 14, 5:20 PM
Closed by commit R120:b7b25010aa58: Migrate QQC1 to QQC2 (authored by Guo Yunhe <i@guoyunhe.me>). · Explain WhyThu, Nov 14, 6:39 PM
This revision was automatically updated to reflect the committed changes.
davidedmundson added inline comments.
applets/digital-clock/package/contents/ui/configTimeZones.qml
21–22

Next plasma release can only depend on Qt 5.12

Please can you change accordingly

applets/systemmonitor/common/contents/ui/ConfigGeneral.qml
134

Please keep this the same as before

i18ncp("Suffix for spinbox (seconds)", "second", "%1 secconds", seconds.toFixed(1));

lbeltrame added a subscriber: lbeltrame.EditedMon, Nov 18, 6:52 AM

This change prevents the SDDM theme from loading. See https://bugs.kde.org/show_bug.cgi?id=414252. The change to QQC2 in the import in Main.qml causes it, it seems.