Differential D28152 Diff 78127 kcmkwin/kwinrules/package/contents/ui/RulesEditor.qml

Changeset View

Standalone View

View Options

kcmkwin/kwinrules/package/contents/ui/RulesEditor.qml

  • This file was added.
1​/*
2​ * Copyright (c) 2020 Ismael Asensio <isma.af@gmail.com>
3​ *
4​ * This program is free software; you can redistribute it and/or
5​ * modify it under the terms of the GNU General Public License as
6​ * published by the Free Software Foundation; either version 2 of
7​ * the License or (at your option) version 3 or any later version
8​ * accepted by the membership of KDE e.V. (or its successor approved
9​ * by the membership of KDE e.V.), which shall act as a proxy
10​ * defined in Section 14 of version 3 of the license.
11​ *
12​ * This program is distributed in the hope that it will be useful,
13​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
14​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15​ * GNU General Public License for more details.
16​ *
17​ * You should have received a copy of the GNU General Public License
18​ * along with this program. If not, see <http://www.gnu.org/licenses/>.
19​ */
20​
21​import QtQuick 2.12
22​import QtQuick.Layouts 1.12
23​import QtQuick.Controls 2.12 as QQC2
24​import org.kde.kirigami 2.10 as Kirigami
25​import org.kde.kcm 1.2
26​
27​
28​ScrollViewKCM {
29​ id: rulesEditor
30​ property var rulesModel: kcm.rulesModel
31​
32​ title: rulesModel.description
33​
34​ header: RowLayout {
35​ id: filterBar
36​ Kirigami.SearchField {
37​ id: searchField
broulikUnsubmitted
Done

There's now a Kirigami.SearchField

broulik: There's now a `Kirigami.SearchField`
38​
39​ Layout.fillWidth: true
40​ placeholderText: i18n("Search...")
41​ focusSequence: "Ctrl+F"
42​
43​ onTextChanged: {
ngrahamUnsubmitted
Done

the list items are back to changing their background on hover, which isn't necessary

ngraham: the list items are back to changing their background on hover, which isn't necessary
iasensioAuthorUnsubmitted
Not Done

Having hoverEnabled: false does not allow to show the delete button on hover only.
The other option is to have it always visible, which is not as a bad as I though, and is indeed cleaner than having the background changing colors

iasensio: Having `hoverEnabled: false` does not allow to show the delete button on hover only. The other…
44​ rulesModel.filter.searchText = text;
ngrahamUnsubmitted
Not Done

Perhaps this should be checked when creating a new rule, or else it looks like there aren't any properties you can apply to the window when creating the rule

ngraham: Perhaps this should be checked when creating a new rule, or else it looks like there aren't any…
iasensioAuthorUnsubmitted
Not Done

I have tried it, but it was a bit messy since the editor page does not differentiate when it's a new rule or any other. If I set it whenever no optional settings are enabled, it gives a confusing feeling with all the rows just appearing and dissapearing on a single check.

Maye a hint like this suffice?

iasensio: I have tried it, but it was a bit messy since the editor page does not differentiate when it's…
45​ }
46​ }
47​ QQC2.ToolButton {
48​ id: showAllButton
49​ icon.name: checked ? 'view-visible' : 'view-hidden'
ngrahamUnsubmitted
Done

Might be a good idea to move the "Add properties... button right below this text so it's obvious to the user now they add more properties. When the placeholder message is not shown, the button could just be there at the bottom of the list, and maybe could change its text to "Add more properties..."

The reason for this is to make it easy for the user to understand what they need to do. We do this in the online accounts KCM:

ngraham: Might be a good idea to move the "Add properties... button right below this text so it's…
50​ text: i18n("Show All Rules")
51​ checkable: true
52​ enabled: searchField.text.trim() == ""
53​ Binding {
54​ target: rulesModel.filter
55​ property: "showAll"
56​ value: showAllButton.checked
57​ }
58​ }
59​ }
60​
61​ view: ListView {
62​ id: rulesView
broulikUnsubmitted
Done

I think this needs to use title capitalization: "Show All Rules"

broulik: I think this needs to use title capitalization: "Show All Rules"
63​ clip: true
64​
65​ model: rulesModel.filter
66​ delegate: RuleItemDelegate {}
67​ section {
broulikUnsubmitted
Done

You could also us a Binding for that:

Binding {
    target: rulesModel.filter
    property: "showAll"
    value: showAllButton.checked
}
broulik: You could also us a `Binding` for that: ``` Binding { target: rulesModel.filter…
68​ property: "section"
69​ delegate: Kirigami.ListSectionHeader { label: section }
70​ }
71​ }
72​
73​ // FIXME: InlineMessage.qml:241:13: QML Label: Binding loop detected for property "verticalAlignment"
74​ footer: GridLayout {
ngrahamUnsubmitted
Done

I would make this footer a header to mimic the natural flow. Typically you'll click the "detectwindow properties" button first to target a window, which means it should be at the top of the window to suggest that it's the first thing you want to click on.

ngraham: I would make this footer a header to mimic the natural flow. Typically you'll click the…
75​ id: kcmFooter
76​ columns: 2
ngrahamUnsubmitted
Done

Button titles use Title Case ("Detect Window Properties")

ngraham: Button titles use Title Case ("Detect Window Properties")
77​
78​ QQC2.Button {
79​ Layout.fillWidth: true
80​ text: i18n("Detect window properties")
81​ icon.name: "edit-find" // TODO: Better icon for "Detect window properties"
82​ onClicked: {
83​ rulesModel.detectWindowProperties(detection_delay.value);
84​ }
85​ }
ngrahamUnsubmitted
Done

In English at least, this minimum width isn't high enough to avoid the spinbox getting wider when going from 1 to 2.

ngraham: In English at least, this minimum width isn't high enough to avoid the spinbox getting wider…
86​
87​ QQC2.SpinBox {
88​ id: detection_delay
89​ from: 0
90​ to: 30
zzagUnsubmitted
Done

Use camelCase please.

zzag: Use camelCase please.
ngrahamUnsubmitted
Done

Same deal for the singular case here; use %1 instead of 1

ngraham: Same deal for the singular case here; use `%1` instead of `1`
91​ textFromValue: (value, locale) => {
92​ return (value == 0) ? i18n("Instantly")
93​ : i18np("After 1 second", "After %1 seconds", value)
94​ }
95​ }
96​
97​ Kirigami.InlineMessage {
zzagUnsubmitted
Not Done

If an implementation for textFromValue is provided, then we also must provide an implementation for valueFromText.

zzag: If an implementation for textFromValue is provided, then we also must provide an implementation…
iasensioAuthorUnsubmitted
Not Done

The spinbox is not editable, I would just copy the default implementation.
It's also required in this case?

iasensio: The spinbox is not editable, I would just copy the default implementation. It's also required…
ngrahamUnsubmitted
Done

That's correct; it's not required unless the spinbox is editable.

ngraham: That's correct; it's not required unless the spinbox is editable.
zzagUnsubmitted
Not Done

Hmm, okay. I just restated what Qt docs say.

zzag: Hmm, okay. I just restated what Qt docs say.
ngrahamUnsubmitted
Done

use Title Case and add ellipses ("Add Properties...")

ngraham: use Title Case and add ellipses ("Add Properties...")
98​ id: warningMessage
99​
100​ Layout.columnSpan: kcmFooter.columns
101​ Layout.fillWidth: true
102​ Layout.fillHeight: true
103​
broulikUnsubmitted
Done

Capitalize

broulik: Capitalize
104​ visible: rulesModel.showWarning
105​ text: i18n("You have specified the window class as unimportant.\n" +
106​ "This means the settings will possibly apply to windows from all " +
107​ "applications. If you really want to create a generic setting, it is " +
108​ "recommended you at least limit the window types to avoid special window " +
109​ "types.")
110​ }
111​ }
112​}
broulikUnsubmitted
Done

You can't do word puzzles with i18n like this. It needs to be one consecuetive string.

broulik: You can't do word puzzles with `i18n` like this. It needs to be one consecuetive string.
iasensioAuthorUnsubmitted
Done

I will probably set this text on the C++ side as you suggested in another comment.
This feels a bit alien here.

iasensio: I will probably set this text on the C++ side as you suggested in another comment. This feels a…
ngrahamUnsubmitted
Done

always manually set width to the list view's width on delegates (e.g. width: overlayModel.width)

ngraham: always manually set width to the list view's width on delegates (e.g. `width: overlayModel.
ngrahamUnsubmitted
Done

Needs to be vertically centered, or else this happens:

(probably do the same for other items in this rowlayout as right now their being vertically centered is by chance rather than done explicitly)

ngraham: Needs to be vertically centered, or else this happens: {F8245478} (probably do the same for…
ngrahamUnsubmitted
Done

This doesn't do what you think it does.

searchField.focus doesn't mean, "focus the search field", it means "allow the search field to *become* focused. The property is terribly mis-named. :(

You probably want to set focus: true on the search field itself, and do searchField.forceActiveFocus() here.

Then again I would imagine that the search field internally has focus: true set so setting it again here may be unnecessary.

ngraham: This doesn't do what you think it does. `searchField.focus` doesn't mean, "focus the search…
davidedmundsonUnsubmitted
Not Done

searchField.focus doesn't mean, "focus the search field", it means "allow the search field to *become* focused. The property is terribly mis-named. :(

That's not right.

focus does mean "become focussed".

However, it means "become focussed within the parent focus scope".

Focus Scopes are a super simple concept that are simultaneously very very very confusing, to the point that I can't explain them well.

In this case ListView is our focus scope, so we also need to make the listview focussed in order for it to have any effect.

forceActiveFocus() iterates through the tree doing that.

davidedmundson: >searchField.focus doesn't mean, "focus the search field", it means "allow the search field to…
ngrahamUnsubmitted
Not Done

due to a Qt bug, left and right margins are not reversed in RTL mode. So whenever you set one of those, yo need to explicitly handle the RTL case like this:

Layout.leftMargin: !LayoutMirroring.enabled? Kirigami.Units.smallSpacing : 0
Layout.rightMargin: LayoutMirroring.enabled? Kirigami.Units.smallSpacing : 0

(same for all other instances of using left and right margins on an item in a layout)

ngraham: due to a Qt bug, left and right margins are not reversed in RTL mode. So whenever you set one…
davidedmundsonUnsubmitted
Not Done

We shouldn't be working round Qt bugs.

Also..if you do that, what will happen when Qt does fix it?

davidedmundson: We shouldn't be working round Qt bugs. Also..if you do that, what will happen when Qt does…
iasensioAuthorUnsubmitted
Not Done

As a kind of middle way, I set the two margins to the same value

iasensio: As a kind of middle way, I set the two margins to the same value
davidedmundsonUnsubmitted
Not Done

good solution :)

FYI: https://codereview.qt-project.org/c/qt/qtdeclarative/+/297558

davidedmundson: good solution :) FYI: https://codereview.qt-project.org/c/qt/qtdeclarative/+/297558
ngrahamUnsubmitted
Done

Is there a reason to use opacity rather than visible here?

ngraham: Is there a reason to use `opacity` rather than `visible` here?
iasensioAuthorUnsubmitted
Not Done

It is to preserve the space of the button in the Layout when hidden

iasensio: It is to preserve the space of the button in the Layout when hidden
iasensioAuthorUnsubmitted
Not Done

I'm not too proud of this, but it was required to avoid this overlapping

iasensio: I'm not too proud of this, but it was required to avoid this overlapping {F8245978}