Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

plasma-workspace: add systementry sessionmanagement patch #99582

Closed
wants to merge 1 commit into from
Closed

plasma-workspace: add systementry sessionmanagement patch #99582

wants to merge 1 commit into from

Conversation

picnoir
Copy link
Member

@picnoir picnoir commented Oct 4, 2020 •
edited

Motivation for this change

Systemd v246 hides the interface used by KDE to fetch the user session
details. We're applying this commit from 5.19.90 upgrading the
systementry applet to use the new sessionmanagement API.

Because of this API breakage, the "Switch User" item was absent from
the KDE application laucher.

See the #98141 issue for the
full story.

Things done

Pick https://invent.kde.org/plasma/plasma-workspace/-/commit/05414ed58d43d87d907326636faac53ae2e7bd60 from upstream.

The missing items are appearing again and are fully functional.

Screenshot showing the "switch user" item re-appearing in the launch menu

How to Test?

I used this trick #98141 (comment).

Don't forget to point to your local nixpkgs checkout:

mid=$(nixops create nixops-kde-test.nix)
nixops modify -d $mid -I nixpkgs=/path/to/your/checkout nixops-kde-test.nix
nixops deploy -d $mid

Note for @ttuegel: I'll let you handle the backport (if you're ok with this PR ofc :)).

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@picnoir picnoir requested a review from ttuegel October 4, 2020 18:03
@picnoir picnoir added the 9.needs: port to stable A PR needs a backport to the stable release. label Oct 4, 2020
@picnoir picnoir mentioned this pull request Oct 4, 2020
Closed
ttuegel
ttuegel requested changes Oct 4, 2020
View reviewed changes
Copy link
Member

@ttuegel ttuegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use fetchpatch instead of fetchurl.

pkgs/desktops/plasma-5/plasma-workspace/default.nix Outdated Show resolved Hide resolved
pkgs/desktops/plasma-5/plasma-workspace/default.nix Outdated Show resolved Hide resolved
@ttuegel
Copy link
Member

ttuegel commented Oct 4, 2020

@NinjaTrappeur Does this also fix the lock screen?

@picnoir
plasma-workspace: add systementry sessionmanagement patch
a054b43 
Systemd v246 hides the interface used by KDE to fetch the user session
details. We're applying this commit from 5.19.90 upgrading the
systementry applet to use the new sessionmanagement API.

Because of this API breakage, the "Switch User" item was absent from
the KDE application laucher.

See the NixOS#98141 issue for the
full story.
@picnoir
Copy link
Member Author

picnoir commented Oct 4, 2020 •
edited

Does this also fix the lock screen?

I did not check the lock screen, I only verified the item was appearing in the login screen and application launcher.

I destroyed the VM and garbage collected the store. I think I'll call it a day for now.

I'll check this tomorrow night CEST (if you cannot do it by then).

PTAL.

@ttuegel
Copy link
Member

ttuegel commented Oct 4, 2020

The launcher is fixed, but the lock screen is not fixed. I guess that we need a patch to kscreenlocker, too.

@worldofpeace worldofpeace added this to To Do in 20.09 Blockers via automation Oct 4, 2020
@worldofpeace worldofpeace moved this from To Do to In progress in 20.09 Blockers Oct 4, 2020
@worldofpeace worldofpeace moved this from In progress to Needs Review in 20.09 Blockers Oct 4, 2020
jonringer
jonringer reviewed Oct 4, 2020
View reviewed changes
pkgs/desktops/plasma-5/plasma-workspace/default.nix
Comment on lines +49 to +60
( # Systemd v246 hides the interface used by KDE to fetch the
# user session details.
# We're applying this commit from 5.19.90 upgrading the
# systementry applet to use the new sessionmanagement API.
# See https://github.com/NixOS/nixpkgs/issues/98141 for more
# details.
# /!\ Remove this patch for version >= v5.19.90
fetchpatch {
name = "0003-port-systementry-to-sessionmanagement-api.patch";
url = "https://invent.kde.org/plasma/plasma-workspace/-/commit/05414ed58d43d87d907326636faac53ae2e7bd60.patch";
sha256 = "16izfcwxjkdn99j777ywjkzl01iyl542h4hpsbpkckccj7hz8sin";
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be hacky since there's not a good "Qt source of truth", but would we be able to conditionally disable this, something like:

] ++ lib.optional (lib.versionOlder qtquickcontrols.version "5.19") [
  <patch>
 ];

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, the Qt PRs will probably visit this, and might not be necessary.

Too much time spent doing python PRs :)

@jonringer
Copy link
Contributor

seems like no regressions from a program standpoint. Haven't tested

https://github.com/NixOS/nixpkgs/pull/99582
3 packages marked as broken and skipped:
libsForQt5.khotkeys libsForQt512.khotkeys libsForQt515.khotkeys

16 packages built:
kde-cli-tools kdeplasma-addons kdev-php kdev-python kdevelop kdevelop-unwrapped kmenuedit krohnkite kwin-dynamic-workspaces kwin-tiling plasma5.khotkeys plasma-desktop plasma-workspace powerdevil systemsettings wacomtablet

@picnoir
Copy link
Member Author

picnoir commented Oct 5, 2020

Indeed, I can also reproduce the problem on the lockscreen. I don't see any fix upstream be it in code or as a related bugtracker ticket.

I need help here, there's a hole in my understanding of kde-workspace.

As far I understand, the item visibility is defined at https://invent.kde.org/plasma/plasma-workspace/-/blob/master/lookandfeel/contents/lockscreen/LockScreenUi.qml#L277

The sessionsModel.canSwitchUser is the right way to get the capability, I don't see anything wrong :/

However, this action item is clearly not visible. Meaning either sessionsModel.canStartNewSession or sessionsModel.canSwitchUser is not verified in our context.

Any idea what could happen here @ttuegel? Are you in contact with some upstream dev. They might be able to help here.

I'd be interested to see if your systemd patch fixes this issue. If it is, I guess we should investigate the sessionsModel.canSwitchUser implementation.

@worldofpeace worldofpeace removed this from Needs Review in 20.09 Blockers Oct 5, 2020
@picnoir
Copy link
Member Author

picnoir commented Oct 5, 2020

Fixed by #99629

@picnoir picnoir closed this Oct 5, 2020
@picnoir picnoir deleted the nin-fix-plasma-workspace branch October 5, 2020 18:37
@flokli flokli mentioned this pull request Oct 26, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer jonringer left review comments

@ttuegel ttuegel ttuegel requested changes

Assignees
No one assigned
Labels
6.topic: qt/kde 9.needs: port to stable A PR needs a backport to the stable release. 10.rebuild-darwin: 0 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@picnoir @ttuegel @jonringer