[sddm-theme] Start moving from QQC1 to QQC2
AcceptedPublic

Authored by cblack on Jun 15 2019, 2:33 AM.

Details

Reviewers
davidedmundson
filipf
Group Reviewers
Plasma
VDG
Maniphest Tasks
T10958: Faster Startup
Summary

Components that used QQC1 in sddm-theme/ (excluding the symlinked components folder) have been changed to use QQC2.

Test Plan

Ensure no loss of functionality, style, or bugs from the port of QQC1 to QQC2.

Diff Detail

Repository
R120 Plasma Workspace
Branch
sddm-qqc2-port (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13019
Build 13037: arc lint + arc unit
cblack created this revision.Jun 15 2019, 2:33 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 15 2019, 2:33 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
cblack requested review of this revision.Jun 15 2019, 2:33 AM
cblack updated this revision to Diff 59842.Jun 15 2019, 2:34 AM

Looks like this change managed to hide from Arcanist's gaze. Should have been there in the first place.

cblack edited the summary of this revision. (Show Details)Jun 15 2019, 2:36 AM
davidedmundson requested changes to this revision.Jun 15 2019, 6:38 AM
davidedmundson added a subscriber: davidedmundson.

Concept makes sense, minor tweaks above

Make sure you test in full sddm, the test mode will infer some settings from your session which won't be valid in real usage.

sddm-theme/KeyboardButton.qml
22–26

Please fix.

sddm-theme/SessionButton.qml
27

Why the wrapper?

29

Implicit height should be propagated upwards, not height.

This revision now requires changes to proceed.Jun 15 2019, 6:38 AM
cblack updated this revision to Diff 59880.Jun 15 2019, 3:20 PM

Updates based off of feedback, and adding the standard menu fade animation to the QQC2 menus.

cblack added inline comments.Jun 15 2019, 3:22 PM
sddm-theme/SessionButton.qml
27

QML files can only have one root, and the ToolButton's menu property doesn't accept QQC2 menus, meaning they have to be separated.

filipf added a subscriber: filipf.Jun 15 2019, 4:38 PM

Seems like you have all the QQC1 imports covered. I'm seeing a couple more in other files that are unused so it would be perfectly fine to remove them as a part of this diff.

I'll give the new code a spin soon and then I can add more detailed comments.

davidedmundson added inline comments.Jun 15 2019, 4:39 PM
sddm-theme/SessionButton.qml
23

Note that (confusingly) Plasma components 2 is QQC1
Plasma components3 is QQC2

It might fix the menu issue

29

I meant

implicitWidth: toolBtn.implicitWidth

width by default is bound to the implicitWidth so you don't need to set it
but more importantly you still have the data of the size something wants to be after someone else (a layout) sets the size

cblack added inline comments.Jun 15 2019, 4:43 PM
sddm-theme/SessionButton.qml
23

The menu property seems to be removed in PC3

cblack added inline comments.Jun 15 2019, 4:48 PM
sddm-theme/SessionButton.qml
23

Additionally, ToolButton still seems to use QQC1 even in PC3.

cblack updated this revision to Diff 59884.Jun 15 2019, 4:54 PM

Use PC3 components in the kb & session buttons instead of PC2 components

Added some comments that apply to both button files.

On a final note, watch out for whitespace because there's lots of it that's been added. You can turn on whitespace displaying in Kate:

sddm-theme/SessionButton.qml
27

It seems to be running fine without the Item wrapper for me. What I did though was but the QQC2 menu inside the button, I didn't split them up.

43

While we're doing this, can you perhaps make the menu pop up *above* the button?

56

This can be up for debate, but I sort of liked the older behavior more when the menu's width would be determined by the size of the ToolButton at a particular point.

81

Why do we need to set this?

cblack added inline comments.Jun 20 2019, 2:44 PM
sddm-theme/SessionButton.qml
43

It seems that changing the y value doesn't affect its y position for whatever reason.

cblack updated this revision to Diff 60136.Jun 20 2019, 2:45 PM
  • Remove Item {} root and embed QQC2.Menu {} in PlasmaComponents.ToolButton{}
  • If toolbutton is larger than largest menu item, resize menu to size of toolbutton
  • Other minor things
ngraham added inline comments.
sddm-theme/KeyboardButton.qml
10

Prefer descriptive variable names; toolButton is better than toolBtn, but even better would be a description of what this button is actually for. The original was better and didn't need to be changed IMO.

For positioning the menu maybe you could have a look at how it was done in Kickoff config window: https://github.com/KDE/plasma-desktop/blob/master/applets/kickoff/package/contents/ui/ConfigGeneral.qml

For positioning the menu maybe you could have a look at how it was done in Kickoff config window: https://github.com/KDE/plasma-desktop/blob/master/applets/kickoff/package/contents/ui/ConfigGeneral.qml

The menu seems to be extremely stubborn when it comes to y positioning. No amount of properties set or values passed to popup will get it to budge from its y position, including the method Kickoff uses.

cblack updated this revision to Diff 60379.Jun 22 2019, 4:40 PM
  • Change toolBtn to be more descriptive based on file the id was in
  • Styling changes on menus (they now have shadows)
  • Menu item highlight is tweaked as per T11124 (can revert if shouldn't be here)
filipf added a comment.Jul 3 2019, 9:40 PM

If keeping the new menu style we have to wait until all those patches get merged in Plasma.

Might be better to just follow current style; updating it to the new one would be a quick review.

Might be better to just follow current style; updating it to the new one would be a quick review.

+1

GB_2 added a comment.Aug 13 2019, 8:06 PM

If keeping the new menu style we have to wait until all those patches get merged in Plasma.

Might be better to just follow current style; updating it to the new one would be a quick review.

Yeah, keep the current style.

cblack updated this revision to Diff 63936.Aug 17 2019, 5:02 PM

The new highlight style has been yeeted in favor of the current one

cblack edited the summary of this revision. (Show Details)Aug 17 2019, 5:06 PM
cblack edited the test plan for this revision. (Show Details)

LGTM in use. The only thing I'd ask you to do is add font.pointSize: config.fontSize to both of the QQC2.Labels you were working on. SDDM defaults to 9pt font sizes; we need to override that so it's 10pt (same as the Plasma default).

davidedmundson accepted this revision.Aug 20 2019, 1:20 PM
This revision is now accepted and ready to land.Aug 20 2019, 1:20 PM
cblack updated this revision to Diff 64260.Aug 21 2019, 10:14 PM

Add font.pointSize: config.fontSize

cblack updated this revision to Diff 64261.Aug 21 2019, 10:15 PM

Unscrew indentation

filipf accepted this revision.Aug 21 2019, 10:51 PM

Yo @cblack I think you can land this.

ndavis added a subscriber: ndavis.Sep 16 2019, 8:51 PM

Is this screenshot outdated? Why does the selected item extend outside the width of the list?