QML mouse cursor KCM and components
ClosedPublic

Authored by mart on Nov 7 2017, 1:13 PM.

Details

Summary

introduce a cursor theme kcm ported to QML, following the new
design guidelines, alongside generic components that will be reused for
modules that are mostly big grid views, such as look and feel.

BUG: 375106

Test Plan

tested on different devices with different dpi

Diff Detail

Repository
R119 Plasma Desktop
Branch
kcm-redesign/cursorTheme
Lint
No Linters Available
Unit
No Unit Test Coverage
mart created this revision.Nov 7 2017, 1:13 PM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 7 2017, 1:13 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart updated this revision to Diff 22024.Nov 7 2017, 1:18 PM
  • use Q_DECL_OVERRIDE
mart updated this revision to Diff 22026.Nov 7 2017, 1:19 PM
  • make use of the separed kcm modules
  • use Q_DECL_OVERRIDE
romangg added a subscriber: romangg.EditedNov 7 2017, 1:39 PM
  • Open the kcm with kcmshell5 kcm_cursortheme opens it in a very small view with only one column
  • There is no white background to the list view

ngraham added a subscriber: ngraham.EditedNov 7 2017, 1:54 PM

Even though that window is narrow, it looks like we have room for another column; we should use it. If this view is capable of resizing itself to fit in very narrow windows, that's great, and it will make a bit difference in letting System Settings be more compact over time, which is important because the default layout has an omnipresent left sidebar that takes up space.

mart added a comment.Nov 10 2017, 10:42 AM
In D8692#165286, @subdiff wrote:
  • Open the kcm with kcmshell5 kcm_cursortheme opens it in a very small view with only one column

is the case of just few pixels too little to do a second column, to fix it it would need for thumbnails to be resizable...

perhaps i should make thumbnails flexible when the view is small enough and wouldn't let 2 columns

  • There is no white background to the list view

the white background depends from https://phabricator.kde.org/D8526

mart updated this revision to Diff 22142.Nov 10 2017, 11:30 AM
  • ensure at least 2 columns are visible
mart added a comment.Nov 10 2017, 11:31 AM
In D8692#165286, @subdiff wrote:
  • Open the kcm with kcmshell5 kcm_cursortheme opens it in a very small view with only one column

can you try it now with the last commit?

januz added a subscriber: januz.Nov 10 2017, 1:18 PM
ngraham edited the summary of this revision. (Show Details)Nov 10 2017, 11:20 PM

Does it add a horizontal scrollbar when the width is not enough for all the pushbuttons, like in the QWidget-based KCM (screenshot below)?

mart added a comment.Nov 16 2017, 3:03 PM

Does it add a horizontal scrollbar when the width is not enough for all the pushbuttons, like in the QWidget-based KCM (screenshot below)?

this screenshot if from the old module, you have to actually unistall it or will keep loading that one

Does it add a horizontal scrollbar when the width is not enough for all the pushbuttons, like in the QWidget-based KCM (screenshot below)?

this screenshot if from the old module, you have to actually unistall it or will keep loading that one

I know. The question was about the new KCM. Some buttons may become hard to discover if there is no scroolbar anymore.

A scrollbar to show hidden buttons is a pretty bad UX. We should aim for something better in the new version. Perhaps the buttons should lose their text and become icons-only when the window is too narrow to show them all with icons and text.

mart added a comment.Nov 16 2017, 3:33 PM

A scrollbar to show hidden buttons is a pretty bad UX. We should aim for something better in the new version. Perhaps the buttons should lose their text and become icons-only when the window is too narrow to show them all with icons and text.

or perhaps relayout to 2 lines...

mart updated this revision to Diff 22529.Nov 17 2017, 3:45 PM
  • internal GridView implementation

put the whole logic for cell sizes and the background painting as well in a
separed control (todo: will then need to be different in plasma mobile)

In D8692#168596, @mart wrote:

or perhaps relayout to 2 lines...

Yes, that would work too.

mart updated this revision to Diff 22534.Nov 17 2017, 4:12 PM
  • two rows toolbar if needed
mart updated this revision to Diff 22649.Nov 20 2017, 1:25 PM
  • don't implement own background
davidedmundson added inline comments.
kcmcontrols/GridView.qml
33

It never ever makes sense for an item's implicitSize to be based on the parent's current size.

It's up to the parent to increase this item's actual width to be bigger than the implicitWidth

kcms/cursortheme/xcursor/thememodel.cpp
305

why change these QStringLiterals? Is a rebase needed?

321

This seems unrelated, adn I'm not sure is right.

mart added inline comments.Nov 20 2017, 2:11 PM
kcms/cursortheme/xcursor/thememodel.cpp
305

yeah, it's so long this review is open that doesn't merge cleanly with master anymore :/

mart updated this revision to Diff 22656.Nov 20 2017, 3:07 PM
  • Merge branch 'master' into kcm-redesign/cursorTheme
  • remove spurious changes to resync with master
mart updated this revision to Diff 22658.Nov 20 2017, 3:10 PM
  • restore Q_DECL_OVERRIDE
mart updated this revision to Diff 22820.Nov 23 2017, 12:01 PM
  • remove spurious changes to resync with master
  • use qqc1 buttons for the icon
  • adapt to new scrollview behavior
  • base components: SimpleKCM GridViewKCM
hein requested changes to this revision.Nov 23 2017, 12:29 PM
hein added a subscriber: hein.
hein added inline comments.
kcms/cursortheme/kcmcursortheme.cpp
150

Incorrect Apply button management

550

Coding style (many other places)

This revision now requires changes to proceed.Nov 23 2017, 12:29 PM
mart updated this revision to Diff 22829.Nov 23 2017, 2:36 PM
  • correctly manage apply
hein added a comment.Nov 24 2017, 8:46 AM

I don't think we can put the components into plasma-desktop, especially SimpleKCM if you really intend it to be used by all KCMs as root item. That would make any KCM requiring something like this depend on plasma-desktop. We have many KCMs in frameworks (e.g. KIO and Sonnet) which can't depend on plasma-desktop. Even working out the tiering in KF5 itself may be a little tricky.

hein added a comment.Nov 24 2017, 8:48 AM

Related to the above: We also have various cases where applications bring up KCMs via kcmshell, e.g. KIO's Web Shortcuts KCM. That means also at runtime none of this can depend on plasma-desktop in any way. KCMs need to work outside of Plasma with a sane dep tree. Styling needs to work outside of Plasma and Breeze, too.

Also - not strictly related to this review, but I did testing for D8911 via kcmshell5, and there's an enormous amout of QML ReferenceErrors going on with Qt Quick KCM code currently.

mart updated this revision to Diff 22884.Nov 24 2017, 10:36 AM
  • port to frameworks-kcmcontrols
This revision was not accepted when it landed; it landed in state Needs Review.Jan 23 2018, 1:40 PM
This revision was automatically updated to reflect the committed changes.