Bug 488954

Would this be a duplicate of Bug 487745?

(In reply to Jakob Petsovits from comment #1)
> Would this be a duplicate of Bug 487745?

Yes, I think so. I didn't find that one when searching before reporting.

There are slight differences in the descriptions; A reboot is not necessary to trigger the issue and the 'Apply button' behaviour is not mentioned.

Shouldn't the initial value of the batteryConservationMode be set to the value read by the getconservationmode job similar to the line above?

https://github.com/KDE/powerdevil/blob/master/kcmodule/profiles/ExternalServiceSettings.cpp#L105

(In reply to Tim from comment #3)
> Shouldn't the initial value of the batteryConservationMode be set to the
> value read by the getconservationmode job similar to the line above?
> 
> https://github.com/KDE/powerdevil/blob/master/kcmodule/profiles/ExternalServiceSettings.cpp#L105

So that it reads like this:

setBatteryConservationMode(m_savedBatteryConservationMode);

which was just written by setSavedBatteryConservationMode() with the job result, instead of

setBatteryConservationMode(m_batteryConservationMode);

which is merely the pre-existing value as it was initialized earlier? I think that's an extremely promising attempt at a fix.

Fabian, could you test this change and submit an MR if it fixes the bug?

Yep, it seems that change fixes it and gives the expected behaviour:

```
--- a/kcmodule/profiles/ExternalServiceSettings.cpp
+++ b/kcmodule/profiles/ExternalServiceSettings.cpp
@@ -102,7 +102,7 @@ void ExternalServiceSettings::load(QWindow *parentWindowForKAuth)
 
         const auto data = job->data();
         setSavedBatteryConservationMode(data.value(QStringLiteral("batteryConservationModeEnabled")).toBool());
-        setBatteryConservationMode(m_batteryConservationMode);
+        setBatteryConservationMode(m_savedBatteryConservationMode);
         m_isBatteryConservationModeSupported = true;
     });
 }
```

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

Git commit 43446c371ba62ab2dd939a242affcd1fd97641e9 by Tim Jagenberg.
Committed on 24/06/2024 at 19:48.
Pushed by jpetso into branch 'master'.

Fix initial UI state of Battery protection setting

The setting 'Battery protection' in System Settings -> Power Management
-> Advanced Power Settings was always initially shown deactivated and
did not reflect the actual state of the system as given in
`/sys/bus/platform/drivers/ideapad_acpi/VPC2004\:00/conservation_mode`.
This was fixed so that the initial GUI state reflects the current system
state.

M  +1    -1    kcmodule/profiles/ExternalServiceSettings.cpp

https://invent.kde.org/plasma/powerdevil/-/commit/43446c371ba62ab2dd939a242affcd1fd97641e9

Git commit 7da820f5ba64766b15c1451ad0fbed3b714ed307 by Jakob Petsovits.
Committed on 24/06/2024 at 19:58.
Pushed by jpetso into branch 'Plasma/6.1'.

Fix initial UI state of Battery protection setting

The setting 'Battery protection' in System Settings -> Power Management
-> Advanced Power Settings was always initially shown deactivated and
did not reflect the actual state of the system as given in
`/sys/bus/platform/drivers/ideapad_acpi/VPC2004\:00/conservation_mode`.
This was fixed so that the initial GUI state reflects the current system
state.


(cherry picked from commit 43446c371ba62ab2dd939a242affcd1fd97641e9)

Co-authored-by: Tim Jagenberg <tim@jagenberg.info>

M  +1    -1    kcmodule/profiles/ExternalServiceSettings.cpp

https://invent.kde.org/plasma/powerdevil/-/commit/7da820f5ba64766b15c1451ad0fbed3b714ed307

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

(In reply to Tim from comment #2)
> (In reply to Jakob Petsovits from comment #1)
> > Would this be a duplicate of Bug 487745?
> 
> Yes, I think so. I didn't find that one when searching before reporting.
> 
> There are slight differences in the descriptions; A reboot is not necessary
> to trigger the issue and the 'Apply button' behaviour is not mentioned.

i didn't mentioned, but i get the same behaviour, i didn't mentioned because i though that the checkbox "syncs" with the system that's why the apply button stay greyed out, so intended behavior i guess?

(In reply to Vinícius from comment #10)
> (In reply to Tim from comment #2)
> > (In reply to Jakob Petsovits from comment #1)
> > > Would this be a duplicate of Bug 487745?
> > 
> > Yes, I think so. I didn't find that one when searching before reporting.
> > 
> > There are slight differences in the descriptions; A reboot is not necessary
> > to trigger the issue and the 'Apply button' behaviour is not mentioned.
> 
> i didn't mentioned, but i get the same behaviour, i didn't mentioned because
> i though that the checkbox "syncs" with the system that's why the apply
> button stay greyed out, so intended behavior i guess?

The incorrect "saved state" would also affect whether the Apply button gets enabled or disabled. Not exactly intended behavior but the fix explains why it would behave incorrectly. I would assume (but can't test on my system) that the fix makes both the checkmark and the button work correctly for both of you.

(In reply to Jakob Petsovits from comment #11)

> The incorrect "saved state" would also affect whether the Apply button gets
> enabled or disabled. Not exactly intended behavior but the fix explains why
> it would behave incorrectly. I would assume (but can't test on my system)
> that the fix makes both the checkmark and the button work correctly for both
> of you.

make send, thank you!, gonna test when it lands of fedora rawhide