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.

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

Various Additions to PowerDevil Configuration #379

Merged
merged 14 commits into from
Oct 16, 2024

Conversation

Green-D-683
Copy link
Contributor

This PR includes various additions to the plasma-manager powerdevil module - for configuring the KDE Plasma Power Management Settings:

  • Display Brightness per power mode
  • Power Profile per Power Mode
  • Battery Level Management
    • Low/Critical Power Levels
    • Critical Power Action

There are also a couple of bugfixes:

  • Display Dimming Idle Timeout was not being set properly - it should be -1 if dimming is disabled
  • Lid Closing actions used "shutdown" in place of the "shutDown" used by all other actions, for the sake of consistency, this option was added alongside "shutdown" - this should probably be a replacement, but I wasn't sure how to write the mkRenamedOption or Assertion to give a reasonable warning as to the replacement having occurred.

@HeitorAugustoLN HeitorAugustoLN added 0. type: enhancement New feature or request for feature 1. scope: powerdevil Related to powerdevil module 4. has: plasma module (update) Updates plasma module labels Oct 10, 2024
@HeitorAugustoLN
Copy link
Member

Didn't test it yet, but I just noticed there some 4 spaces indent in the file. Could you please format it with nix develop && nix fmt?

@Green-D-683 Green-D-683 reopened this Oct 10, 2024
@Green-D-683
Copy link
Contributor Author

Not sure why that closed, but that should be formatted properly now

it will be a simple change, so it doesn't really need backwards compatibility. Since nix will already tell the fix
Copy link
Member

@HeitorAugustoLN HeitorAugustoLN left a comment

Choose a reason for hiding this comment

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

Looks good to me! Did some minor changes to ensure consistency. But I'll let @magnouvean merge this. Since I don't really have a laptop, so I can't test some of these options

@magnouvean
Copy link
Collaborator

I'll try to test it this weekend. Sorry for the longer delays here recently, I've been really busy with work lately.

@magnouvean
Copy link
Collaborator

Mostly looks very good from my testing, but with the displayBrightness I see that when this option is set the "change screen brightness" toggle is still disabled by default. In my opinion this should automatically be enabled when this value is set, since the changing of the display brightness won't have any effect until this toggle is turned on. This should be as simple as setting:

UseProfileSpecificDisplayBrightness=true

in the powerdevilrc. Alternatively it could be added as a separate option if you think it makes sense to configure these two options independently of each other (I'd be fine with either solution tbh). Once this is fixed I think everything is good.

@tucho
Copy link
Contributor

tucho commented Oct 14, 2024

I just opened a PR with changes to powerdevil, changes I had been working on for several days. I hadn't seen that this PR was open. I swear it was not an attempt to compete, I opened it from the fork on my profile, without looking at the project's PR list.

@magnouvean
Copy link
Collaborator

That's fine, your pr seems more general while this is a little more specific, so I think we can merge this first when things are fixed and then resolve conflicts and test/merge yours @tucho :)

@tucho
Copy link
Contributor

tucho commented Oct 15, 2024

Nice! Thank you @magnouvean for your reply.

Copy link
Contributor Author

@Green-D-683 Green-D-683 left a comment

Choose a reason for hiding this comment

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

Mostly looks very good from my testing, but with the displayBrightness I see that when this option is set the "change screen brightness" toggle is still disabled by default. In my opinion this should automatically be enabled when this value is set, since the changing of the display brightness won't have any effect until this toggle is turned on. This should be as simple as setting:

UseProfileSpecificDisplayBrightness=true

in the powerdevilrc. Alternatively it could be added as a separate option if you think it makes sense to configure these two options independently of each other (I'd be fine with either solution tbh). Once this is fixed I think everything is good.

Thank you, I must have missed that when I was developing this, as mine would have been set previously. My config is also partially ported from Plasma 5, so some of the values are still in the old file location, and I will have missed this when I checked that everything in my PowerDevilrc was available as an option.

@magnouvean
Copy link
Collaborator

It works fine now. I'll merge right away, thanks for the contribution :)

@magnouvean magnouvean merged commit 508a077 into nix-community:trunk Oct 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. type: enhancement New feature or request for feature 1. scope: powerdevil Related to powerdevil module 4. has: plasma module (update) Updates plasma module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants