Changeset View
Standalone View
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.14 | ||||
22 | import QtQuick.Layouts 1.14 | ||||
23 | import QtQuick.Controls 2.14 as QQC2 | ||||
24 | import org.kde.kirigami 2.10 as Kirigami | ||||
25 | import org.kde.kcm 1.2 | ||||
26 | import org.kde.kitemmodels 1.0 | ||||
27 | import org.kde.kcms.kwinrules 1.0 | ||||
28 | | ||||
29 | | ||||
30 | ScrollViewKCM { | ||||
31 | id: rulesEditor | ||||
32 | property var rulesModel: kcm.rulesModel | ||||
33 | | ||||
34 | title: rulesModel.description | ||||
35 | | ||||
36 | header: RowLayout { | ||||
37 | id: filterBar | ||||
broulik: There's now a `Kirigami.SearchField` | |||||
38 | Kirigami.SearchField { | ||||
39 | id: searchField | ||||
40 | Layout.fillWidth: true | ||||
41 | horizontalAlignment: Text.AlignLeft | ||||
42 | } | ||||
43 | QQC2.ToolButton { | ||||
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 | |||||
iasensio: Having `hoverEnabled: false` does not allow to show the delete button on hover only.
The other… | |||||
44 | id: showAllButton | ||||
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… | |||||
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 | icon.name: checked ? 'view-visible' : 'view-hidden' | ||||
46 | text: i18n("Show All Rules") | ||||
47 | checkable: true | ||||
48 | enabled: searchField.text.trim() == "" | ||||
49 | } | ||||
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 | } | ||||
51 | | ||||
52 | view: ListView { | ||||
53 | id: rulesView | ||||
54 | clip: true | ||||
55 | | ||||
56 | model: filterModel | ||||
57 | delegate: RuleItemDelegate {} | ||||
58 | section { | ||||
59 | property: "section" | ||||
60 | delegate: Kirigami.ListSectionHeader { label: section } | ||||
61 | } | ||||
62 | } | ||||
broulik: I think this needs to use title capitalization: "Show All Rules" | |||||
63 | | ||||
64 | footer: ColumnLayout { | ||||
65 | id: kcmFooter | ||||
66 | | ||||
67 | // FIXME: InlineMessage.qml:241:13: QML Label: Binding loop detected for property "verticalAlignment" | ||||
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 | Kirigami.InlineMessage { | ||||
69 | Layout.fillWidth: true | ||||
70 | Layout.fillHeight: true | ||||
71 | text: rulesModel.warningMessage | ||||
72 | visible: text != "" | ||||
73 | } | ||||
74 | | ||||
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 | RowLayout { | ||||
76 | Item { | ||||
ngraham: Button titles use Title Case ("Detect Window Properties") | |||||
77 | Layout.fillWidth: true | ||||
78 | } | ||||
79 | | ||||
80 | QQC2.Button { | ||||
81 | text: i18n("Detect window properties") | ||||
82 | icon.name: "edit-find" // TODO: Better icon for "Detect window properties" | ||||
83 | onClicked: { | ||||
84 | rulesModel.detectWindowProperties(delaySpin.value); | ||||
85 | } | ||||
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 | | ||||
88 | QQC2.SpinBox { | ||||
89 | id: delaySpin | ||||
90 | Layout.minimumWidth: Kirigami.Units.gridUnit * 6 | ||||
zzag: Use camelCase please. | |||||
ngraham: Same deal for the singular case here; use `%1` instead of `1` | |||||
91 | from: 0 | ||||
92 | to: 30 | ||||
93 | textFromValue: (value, locale) => { | ||||
94 | return (value == 0) ? i18n("Instantly") | ||||
95 | : i18np("After 1 second", "After %1 seconds", value) | ||||
96 | } | ||||
97 | } | ||||
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… | |||||
The spinbox is not editable, I would just copy the default implementation. iasensio: The spinbox is not editable, I would just copy the default implementation.
It's also required… | |||||
ngraham: That's correct; it's not required unless the spinbox is editable. | |||||
zzag: Hmm, okay. I just restated what Qt docs say. | |||||
ngraham: use Title Case and add ellipses ("Add Properties...") | |||||
98 | } | ||||
99 | } | ||||
100 | | ||||
101 | KSortFilterProxyModel { | ||||
102 | id: filterModel | ||||
103 | sourceModel: rulesModel | ||||
broulik: Capitalize | |||||
104 | | ||||
105 | property bool showAll: showAllButton.checked | ||||
106 | onShowAllChanged: { | ||||
107 | invalidateFilter(); | ||||
108 | } | ||||
109 | | ||||
110 | filterString: searchField.text.trim().toLowerCase() | ||||
111 | | ||||
112 | filterRowCallback: (source_row, source_parent) => { | ||||
113 | var index = sourceModel.index(source_row, 0, source_parent); | ||||
114 | function itemData (role){ | ||||
115 | return sourceModel.data(index, role) | ||||
116 | } | ||||
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. | |||||
I will probably set this text on the C++ side as you suggested in another comment. iasensio: I will probably set this text on the C++ side as you suggested in another comment.
This feels a… | |||||
117 | | ||||
118 | if (filterString != "") { | ||||
119 | return (itemData(RulesModel.NameRole).toLowerCase().includes(filterString) | ||||
120 | || itemData(RulesModel.ValueRole).toString().toLowerCase().includes(filterString)); | ||||
121 | } | ||||
122 | if (showAll) { | ||||
123 | return true; | ||||
124 | } | ||||
125 | return itemData(RulesModel.EnabledRole) | ||||
126 | } | ||||
127 | } | ||||
128 | } | ||||
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. | |||||
ngraham: Needs to be vertically centered, or else this happens: {F8245478}
(probably do the same for… | |||||
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… | |||||
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… | |||||
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… | |||||
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… | |||||
iasensio: As a kind of middle way, I set the two margins to the same value | |||||
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 | |||||
ngraham: Is there a reason to use `opacity` rather than `visible` here? | |||||
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 | |||||
iasensio: I'm not too proud of this, but it was required to avoid this overlapping
{F8245978} |
There's now a Kirigami.SearchField