[kcmkwin/kwindecoration] Rework decorations buttons drag&drop tab
Needs ReviewPublic

Authored by vpilo on Tue, Jan 8, 4:03 PM.

Details

Reviewers
ngraham
davidedmundson
Group Reviewers
VDG
KWin
Summary
  • Increase the size of the mock titlebar to ease dropping
  • Fix drag&drop behavior
  • Fix reset to default
  • Remove border of available buttons grid
  • Move dragging hint to the top

Screenshots @ https://imgur.com/a/za7CO1I

BUG: 350122
BUG: 346222
BUG: 342816

Test Plan
  • Pressing the Default button after making any changes to the buttons tab correctly results in reset. Also after applying changes.
  • Dragging and dropping over any current button adds the new one to the right of it
  • Dragging and dropping before the leftmost button of a block adds a new first button
  • Dragging and dropping after the rightmost button of a block adds a new last button
  • Dragging and dropping in an empty block adds a first button
  • Moving buttons within a block works as above
  • Moving buttons between blocks works as above

Diff Detail

Repository
R108 KWin
Branch
arcpatch-D18104
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7161
Build 7179: arc lint + arc unit
vpilo created this revision.Tue, Jan 8, 4:03 PM
Restricted Application added a project: KWin. · View Herald TranscriptTue, Jan 8, 4:03 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
vpilo requested review of this revision.Tue, Jan 8, 4:03 PM
vpilo added a subscriber: ngraham.

@ngraham As promised, the reworked Buttons tab :)

abetts added a subscriber: abetts.Tue, Jan 8, 4:08 PM

Do you have a before and after? :D

zzag added a reviewer: KWin.Tue, Jan 8, 4:46 PM

Thanks so much! Will check it out soon. To echo @abetts, before-and-after screenshots in the Summary section are always helpful for patches with UI changes. See https://community.kde.org/Infrastructure/Phabricator#Include_some_screenshots

vpilo edited the summary of this revision. (Show Details)Tue, Jan 8, 8:17 PM
vpilo added a comment.Tue, Jan 8, 8:22 PM

I'm only not very happy with the dragging hint label. It's too dark in dark themes.
Suggestions for an alternative color from the QPalette::ColorRole enum that will work in all themes ? Its opacity is 50% normally and drops to 0% (invisible) during a drag from the titlebar.

vpilo added a comment.Tue, Jan 8, 9:46 PM

And maybe remove the background of the main drag/drop area altogether?

And maybe remove the background of the main drag/drop area altogether?

Yep. Makes sense to me.

Wow, this is amazingly better.

For extra bonus points: could we render an actual depiction of the current theme's titlebar appearance instead of synthesizing an artificial one? The other tab does this in QML, so maybe we can do the same here.

Also +1 on your idea to remove the background color under the unused icons.

vpilo updated this revision to Diff 49115.Wed, Jan 9, 9:42 PM

Review comments: highlight color for the drag hint, background colors fixes

vpilo added a comment.Wed, Jan 9, 9:48 PM

Wow, this is amazingly better.

For extra bonus points: could we render an actual depiction of the current theme's titlebar appearance instead of synthesizing an artificial one? The other tab does this in QML, so maybe we can do the same here.

Also +1 on your idea to remove the background color under the unused icons.

What a ride. I was unable to do neither!
For some reason (different color profile? qml view rendering issue?) the color never matches *exactly* what I set . I don't know. Could you maybe add someone who knows more about colors and QML?

Also I tried making a real window to drag&drop buttons. I went as far as allowing dropping buttons to the fake window's titlebar to add them there, but I couldn't find how to get which button is being dragged from the titlebar. Bummer. It'd have been really nice.

davidedmundson requested changes to this revision.Fri, Jan 11, 2:46 AM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
kcmkwin/kwindecoration/declarative-plugin/buttonsmodel.cpp
169

This still applies.

You're not seeing it because you now add +1 in your QML side, but that makes the guard at the top wrong and the else statement here wrong.

I would suggest adding https://doc.qt.io/qt-5.11/qabstractitemmodeltester.html whilst you're doing dev work and you'll see this crash.

This revision now requires changes to proceed.Fri, Jan 11, 2:46 AM
vpilo updated this revision to Diff 49597.Wed, Jan 16, 8:38 AM
  • Re-add +1 in addRows(), and fix moving buttons to the rightmost position.

Tested with QAbstractItemModelTester in fatal mode, no more issues found.

vpilo marked an inline comment as done.Wed, Jan 16, 8:38 AM

Hey all, sorry for not showing up earlier in this ticket. I wanted to offer visual direction for this change. Here is the mockup I conceptualized for this kcm.

https://phabricator.kde.org/M112/404/

You don't have to follow it strictly, but I think it helps cover the visual changes that we planned. If you could achieve something similar to that, it would be awesome. At a minimum, I agree with the patch that the thumbnails for the dragging items need to be bigger.

+1

vpilo added a comment.Wed, Jan 16, 4:22 PM

@abetts Thank you, I will certainly take in the nice style of the redesign you guys made! It's going to be really easy to use (and cool looking!).

However, since I am currently busy rewriting the entire module with Kirigami, and I am using the current change as the basis for my redesign, it would be more messy for me to make further changes to it at this point. Would it be acceptable to keep it as it is for now?
A couple weeks should be enough for me to come up with the next iteration of this control module, meaning that users of released versions wouldn't see this diff's version (effectively now an 'intermediate' version) at all.

Another question: will all the current settings control modules be restyled like that redesign? Because currently not a single one of the modules look anything like that.

The mockup is a "low fidelity" mockup designed to show layout and structure, not final appearance. You can ignore the visual divergences of the individual controls.