D28152: [RFC] KWinRules KCM Redesign

iasensio added a subscriber: hchain.
iasensio added inline comments.

INLINE COMMENTS

> broulik wrote in kcmrules.cpp:140
> Could this be done in-memory without a temp file?

Yes, I want to talk to @hchain and see to expand the `RuleBookSettings` class to work with a list of `RuleSettings` objects instead of `Rules`, and avoid this intermediate step.

> broulik wrote in OptionsComboBox.qml:41
> Kudos for using `onActivated` rather than `onCurrentIndexChanged`! ;)
> 
> However, you can probably make this a binding
> 
>   readonly property var currentValue: values[index]
> 
> or perhaps a `string` property, even

That approach fails for me (`QCoreApplication::postEvent: Unexpected null receiver`), even with some protections

> broulik wrote in RuleItemDelegate.qml:28
> Needed?

Not really. In fact keyboard navigation works slightly better without setting `focus`

> broulik wrote in RuleItemDelegate.qml:67
> I think you better set that on the `Label` and remove this item, so the description can always span the full width?

This helps keeping the description sizes more constant between delegates, so it looks more like a real table.

Setting `Layout.fillWidth:true` on the `Label`produces this
F8186870: Screenshot_20200320_233750.png <https://phabricator.kde.org/F8186870>

> broulik wrote in RulesEditor.qml:115
> 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.
This feels a bit alien here.

> broulik wrote in RulesList.qml:42
> I think it does that for you already?

Quite, but not fully. If `clip` is not set here, the content seems to overlap the frame. 
Maybe that's a bug in `ScrollViewKCM`.

> broulik wrote in ruleitem.h:39
> I /think/ if you based your class on `QMetaObject::Type` you can generalize it a lot without putting `switch` statements for your types in it? However, there's also `Coordinate` and `Shortcut`, so maybe not.

I definitely want to go with a nicer approach for this thing, but for now is the simplest way I found. @hchain suggested to use a template, ideally, using the type on the KConfigXT  schema.

Currently `RuleItem::Type` has a double meaning: it sets the stored type and the edit field in the UI. And anyway, `Coordinate` just maps to a `QPoint` or `QSize`, and `Shortcut` a `QKeySequence`.

> broulik wrote in rulesdialog.cpp:34
> Where is this class being used from?

It's called by kwin when selecting `Edit Specific Window/App Properties` from the window menu. Here I just kept the interface with that entry point unchanged, but it is within a custom executable, so it should probably be replaced by a full QML dialog by changing `main.cpp`.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D28152

To: iasensio, #plasma, #kwin, #vdg
Cc: hchain, broulik, zzag, kwin, dmenig, manueljlin, Orage, cacarry, LeGast00n, The-Feren-OS-Dev, cblack, konkinartem, ian, jguidon, Ghost6, jraleigh, zachus, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, mkulinski, trickyricky26, ragreen, jackyalcine, iodelay, crozbo, ndavis, bwowk, ZrenBot, firef, ngraham, alexeymin, skadinna, himcesjf, lesliezhai, ali-mohamed, hardening, romangg, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, ahiemstra, mbohlender, mart