Is this that thing you fixed recently, Jakob?

(In reply to Nate Graham from comment #1)
> Is this that thing you fixed recently, Jakob?

Possibly, but the reproduction steps don't involve *closing* navigating away from the Energy Saving KCM (by closing System Setting, or by trying to open another settings module), which was a crucial step in Bug 481045.

Tom, could you confirm that merely opening Advanced Power Settings is causing the crash? i.e. you don't even get to see the settings form with battery levels and stuff?

(In reply to Jakob Petsovits from comment #2)
> (In reply to Nate Graham from comment #1)
> > Is this that thing you fixed recently, Jakob?
> 
> Possibly, but the reproduction steps don't involve *closing* navigating away
> from the Energy Saving KCM (by closing System Setting, or by trying to open
> another settings module), which was a crucial step in Bug 481045.
> 
> Tom, could you confirm that merely opening Advanced Power Settings is
> causing the crash? i.e. you don't even get to see the settings form with
> battery levels and stuff?

yeah as fast as i press the button it crashes, no window even slightly starts to appear like its trying to load. just instantly on press

Thanks. That's not a duplicate then, different bug.

The stacktrace indicates that it wants something from PowerButtonActionModel but is apparently not finding it there, and crashes instead when trying to access it. The only use of PowerButtonActionModel on the Advanced Settings page is for the critical-battery action combo box. Which... hm, here's an idea

In your $HOME/.config/powerdevilrc file, is there a line that goes like,

BatteryCriticalAction=(some value)

If so, could you please paste that line? And also try if it still crashes when you delete it and save the config file?

(In reply to Jakob Petsovits from comment #4)
> Thanks. That's not a duplicate then, different bug.
> 
> The stacktrace indicates that it wants something from PowerButtonActionModel
> but is apparently not finding it there, and crashes instead when trying to
> access it. The only use of PowerButtonActionModel on the Advanced Settings
> page is for the critical-battery action combo box. Which... hm, here's an
> idea
> 
> In your $HOME/.config/powerdevilrc file, is there a line that goes like,
> 
> BatteryCriticalAction=(some value)
> 
> If so, could you please paste that line? And also try if it still crashes
> when you delete it and save the config file?

[BatteryManagement]
BatteryCriticalAction=1

and yeah removing it makes it work again

(In reply to Tom Englund from comment #5)
> [BatteryManagement]
> BatteryCriticalAction=1
> 
> and yeah removing it makes it work again

Okay, nice. "1" is the value for "Sleep". What it looks like to me is that your system does not actually support the Sleep action (as in PowerManagement::canSuspend()), so it's missing from the list of combo box actions but the UI still tries to find and set it. One instance I could see this happening is if one copies an existing laptop Arch installation (with that setting already set) to a new drive that powers e.g. a PC without "Sleep" functionality. Or maybe "Sleep" was the default a long time ago and written to the config file before we used KConfigXT even for global settings (i.e. before my time here).

Either way, we should probably do something more graceful with unsupported power actions than just crash. Although I'm not quite sure how to deal with that in the UI.

For anyone to reproduce a crash like this, one can add this to their ~/.config/powerdevilrc:

[BatteryManagement]
BatteryCriticalAction=16

which is an action that generally exists (PromptLogoutDialog) but does not exist in the option list for the critical battery action combobox.

(In reply to Jakob Petsovits from comment #6)
> (In reply to Tom Englund from comment #5)
> > [BatteryManagement]
> > BatteryCriticalAction=1
> > 
> > and yeah removing it makes it work again
> 
> Okay, nice. "1" is the value for "Sleep". What it looks like to me is that
> your system does not actually support the Sleep action (as in
> PowerManagement::canSuspend()), so it's missing from the list of combo box
> actions but the UI still tries to find and set it. One instance I could see
> this happening is if one copies an existing laptop Arch installation (with
> that setting already set) to a new drive that powers e.g. a PC without
> "Sleep" functionality. Or maybe "Sleep" was the default a long time ago and
> written to the config file before we used KConfigXT even for global settings
> (i.e. before my time here).
> 
> Either way, we should probably do something more graceful with unsupported
> power actions than just crash. Although I'm not quite sure how to deal with
> that in the UI.
> 
> For anyone to reproduce a crash like this, one can add this to their
> ~/.config/powerdevilrc:
> 
> [BatteryManagement]
> BatteryCriticalAction=16
> 
> which is an action that generally exists (PromptLogoutDialog) but does not
> exist in the option list for the critical battery action combobox.

I think its because i disabled sleep in systemd confs, its half broken on my Nvidia dgpu and the kids have fingered that fn + sleep button more then once heh

This issue would also affect regular profile settings btw. It's somewhat mitigated by the fact that many people use default values for their power actions, and in PowerDevil on Plasma 6 we don't have a config file line for settings with default values. So this will only occur if the power action was explicitly set to a non-default value before, and is now unsupported for whatever reason.

I figure what might work is a warning message or dialog that pops up if we can't find a configured power action in an option list anymore. Something like,

"The action you had configured to occur [after a period of inactivity | when the power button is pressed | when the laptop lid is closed | when the battery reaches a critical level] is not supported on your system. Please review your settings and select a different one."

*** Bug 482673 has been marked as a duplicate of this bug. ***

(In reply to Jakob Petsovits from comment #6)
> (In reply to Tom Englund from comment #5)
> > [BatteryManagement]
> > BatteryCriticalAction=1
> > 
> > and yeah removing it makes it work again
> 
> Okay, nice. "1" is the value for "Sleep". What it looks like to me is that
> your system does not actually support the Sleep action (as in
> PowerManagement::canSuspend()), so it's missing from the list of combo box
> actions but the UI still tries to find and set it. One instance I could see
> this happening is if one copies an existing laptop Arch installation (with
> that setting already set) to a new drive that powers e.g. a PC without
> "Sleep" functionality. Or maybe "Sleep" was the default a long time ago and
> written to the config file before we used KConfigXT even for global settings
> (i.e. before my time here).
> 
> Either way, we should probably do something more graceful with unsupported
> power actions than just crash. Although I'm not quite sure how to deal with
> that in the UI.
> 
> For anyone to reproduce a crash like this, one can add this to their
> ~/.config/powerdevilrc:
> 
> [BatteryManagement]
> BatteryCriticalAction=16
> 
> which is an action that generally exists (PromptLogoutDialog) but does not
> exist in the option list for the critical battery action combobox.

i guess worth mentioning 16 didnt work either, removing the entry the only two options i have is "do nothing and shutdown"

(In reply to Tom Englund from comment #10)
> i guess worth mentioning 16 didnt work either

Sorry yeah, 16 was meant as an example for people who explicitly want to make it crash but don't have your system setup.

(In reply to Jakob Petsovits from comment #11)
> (In reply to Tom Englund from comment #10)
> > i guess worth mentioning 16 didnt work either
> 
> Sorry yeah, 16 was meant as an example for people who explicitly want to
> make it crash but don't have your system setup.

oh i misread your comment "For anyone to reproduce a crash like this" i read it first as "for anyone having a crash like this one can add this to their config " gotta properly read the entire thing before thinking! my mistake. :)

A possibly relevant merge request was started @ https://invent.kde.org/plasma/powerdevil/-/merge_requests/337

Git commit 8ecf6430ee7a3d3670fd9cb1b6af947f9ac2f352 by Jakob Petsovits.
Committed on 14/03/2024 at 03:19.
Pushed by jpetso into branch 'master'.

kcmodule: Don't crash when a configured button action is unsupported

The bounds check in the data() method of PowerButtonActionModel
and SleepModeModel was incomplete, so passing an invalid index
such as -1 (standard value for "not found") could cause a crash.

For power management settings, a user would run into this crash if
their configuration from powerdevilrc listed a power button action
that's not supported on the current system. For example, a user
may have selected "Sleep" as lid action, but subsequently changed
their systemd configuration to disable suspend-to-RAM because it
caused problems. Or they could have migrated their OS to a new
computer which does not support Sleep.

Fixing this allows us to assign currentIndex declaratively
instead of waiting until Component.onCompleted to (hopefully) find
a valid item in the model.

M  +1    -1    kcmodule/common/PowerButtonActionModel.cpp
M  +1    -1    kcmodule/common/SleepModeModel.cpp
M  +2    -3    kcmodule/profiles/ui/GlobalConfig.qml
M  +8    -12   kcmodule/profiles/ui/ProfileConfig.qml

https://invent.kde.org/plasma/powerdevil/-/commit/8ecf6430ee7a3d3670fd9cb1b6af947f9ac2f352

Git commit aedb2529b31ec9b62ee5a8b3548be848769f4009 by Jakob Petsovits.
Committed on 14/03/2024 at 03:28.
Pushed by jpetso into branch 'Plasma/6.0'.

kcmodule: Don't crash when a configured button action is unsupported

The bounds check in the data() method of PowerButtonActionModel
and SleepModeModel was incomplete, so passing an invalid index
such as -1 (standard value for "not found") could cause a crash.

For power management settings, a user would run into this crash if
their configuration from powerdevilrc listed a power button action
that's not supported on the current system. For example, a user
may have selected "Sleep" as lid action, but subsequently changed
their systemd configuration to disable suspend-to-RAM because it
caused problems. Or they could have migrated their OS to a new
computer which does not support Sleep.

Fixing this allows us to assign currentIndex declaratively
instead of waiting until Component.onCompleted to (hopefully) find
a valid item in the model.


(cherry picked from commit 8ecf6430ee7a3d3670fd9cb1b6af947f9ac2f352)

M  +1    -1    kcmodule/common/PowerButtonActionModel.cpp
M  +1    -1    kcmodule/common/SleepModeModel.cpp
M  +2    -0    kcmodule/profiles/ui/GlobalConfig.qml
M  +8    -0    kcmodule/profiles/ui/ProfileConfig.qml

https://invent.kde.org/plasma/powerdevil/-/commit/aedb2529b31ec9b62ee5a8b3548be848769f4009

Git commit 78415ae0097ad36377b407d3b143589ca8fa8a18 by Jakob Petsovits.
Committed on 16/03/2024 at 15:33.
Pushed by jpetso into branch 'master'.

kcmodule: Revert to binding currentIndex in Component.onCompleted

Commit 8ecf6430 ("Don't crash when a configured button action
is unsupported") caused empty ComboBox selections on initial load.
Turns out `indexOfValue()` can only be used after the
Component.completed() signal is emitted, so a purely declarative
binding worked incorrectly if queried too early by QML.

Therefore, we revert back to assigning a Qt.binding() to
currentIndex in Component.onCompleted.

This commit also adapts PowerProfileModel and its associated
ComboBox in a similar way as the other two models used in this KCM.
Related: bug 483750

M  +1    -1    kcmodule/common/PowerProfileModel.cpp
M  +4    -1    kcmodule/profiles/ui/GlobalConfig.qml
M  +20   -11   kcmodule/profiles/ui/ProfileConfig.qml

https://invent.kde.org/plasma/powerdevil/-/commit/78415ae0097ad36377b407d3b143589ca8fa8a18

(In reply to Jakob Petsovits from comment #16)
> Git commit 78415ae0097ad36377b407d3b143589ca8fa8a18 by Jakob Petsovits.
> Committed on 16/03/2024 at 15:33.
> Pushed by jpetso into branch 'master'.
> 
> kcmodule: Revert to binding currentIndex in Component.onCompleted

Quick note about this patch: due to a messy but paradoxically bug-free cherry-pick of the original fix in this thread, this follow-up fix-for-the-fix is not required on the Plasma/6.0 branch. The original fix already makes it work as intended on Plasma/6.0, unlike master which briefly experienced Bug 483750 until the follow-up was merged.

*** Bug 484566 has been marked as a duplicate of this bug. ***