Skip to content

Power profiles #426

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

Merged
merged 4 commits into from
Apr 11, 2025
Merged

Power profiles #426

merged 4 commits into from
Apr 11, 2025

Conversation

palinek
Copy link
Contributor

@palinek palinek commented Apr 9, 2025

As of now: quick proof-of-concept to agree if #425 implementation would be reasonable from the user perspective and lean enough from developer perspective to include this feature into LXQt.

@stefonarch
Copy link
Member

stefonarch commented Apr 9, 2025

Nice work!
The Readme would need an update, or also a tooltip to inform that power-profiles-daemon is neededto enable it.
immagine

@palinek
Copy link
Contributor Author

palinek commented Apr 10, 2025

to inform that power-profiles-daemon is neededto enable it

Added a disabled action in case the interface is not available.

@tsujan @stefonarch @libalis
Now the implementation is fully non-blocking and also correctly reacts to outer world changes (even the service on DBus appearing/disappearing).

There is still one minor point.... do we want to support the profile names translation somehow? As those texts/names are fully dynamic and provided by the power-profile-daemon, so we have no control what can come in.

@tsujan
Copy link
Member

tsujan commented Apr 10, 2025

do we want to support the profile names translation somehow?

Is there a standard way?

@libalis
Copy link

libalis commented Apr 10, 2025

The power profiles now actually responds to external conditions — great job!
By the way, I’d be in favor of using translations for accessibility reasons, but only if there’s a way to guarantee enough space to prevent text from getting cut off.
The text Power profile should probably also be sent to the LXQt community for translation, right?
Regarding the icon: what is the new one supposed to represent? It’s still colorful and too small to recognize.

@tsujan
Copy link
Member

tsujan commented Apr 10, 2025

It’s still colorful and too small to recognize.

These things are related to icon sets. Apps shouldn't be bothered by them.

@stefonarch
Copy link
Member

stefonarch commented Apr 10, 2025

There is still one minor point.... do we want to support the profile names translation somehow? As those texts/names are fully dynamic and provided by the power-profile-daemon, so we have no control what can come in.

Is there some sort of list of all possible values? It would be nice to have translations, here I've "balanced" and "power-saver" only.
Btw as usual I'd like to ship it with 2.2 ... it's a nice improvement.

@libalis
Copy link

libalis commented Apr 10, 2025

These things are related to icon sets. Apps shouldn't be bothered by them.

As an app developer, you can select a reasonable default.

@tsujan
Copy link
Member

tsujan commented Apr 10, 2025

As an app developer, you can select a reasonable default.

The most relevant icon is chosen here: preferences-system-performance. The shape and color of an icon are determined by the icon set.

@stefonarch
Copy link
Member

As an app developer, you can select a reasonable default.

The most relevant icon is chosen here: preferences-system-performance. The shape and color of an icon are determined by the icon set.

It's also possible to choose a different icon set only for the panel.

@libalis
Copy link

libalis commented Apr 10, 2025

The most relevant icon is chosen here: preferences-system-performance. The shape and color of an icon are determined by the icon set.

Didn't know that was the selected icon, if you want to stick to the xdg standard that's probably a really good choice, sorry.

It's also possible to choose a different icon set only for the panel.

This doesn't work for individual widgets, but no problem, thank you!

@palinek palinek force-pushed the power-profiles branch 2 times, most recently from f8eeb16 to 4c20dca Compare April 11, 2025 07:34
palinek added 3 commits April 11, 2025 09:35
..to leave the under-the-hood algorithm be driven by QPA platform theme.
The power profiles setting is made available by freedesktop's
power-profiles-daemon.
We just provide basic functionality in case DBus interface is available
in runtime.
We have no control over the profile names returned by
power-profiles-daemon.
We have just added basic profile names as seen being used.
@palinek
Copy link
Contributor Author

palinek commented Apr 11, 2025

Added basic translation support -> just profile name strings of power-profiles-daemon I've seen in my setup.
Rebased to current master.

Now, I believe, this is read for final review/merge.

@palinek palinek marked this pull request as ready for review April 11, 2025 07:41
@palinek palinek requested review from stefonarch and tsujan April 11, 2025 07:41
@palinek
Copy link
Contributor Author

palinek commented Apr 11, 2025

Closes #425

@tsujan
Copy link
Member

tsujan commented Apr 11, 2025

Please remove the ts files from the PR.

@palinek
Copy link
Contributor Author

palinek commented Apr 11, 2025

What's the problem with updated ts files? @stefonarch would update it the same way.

@tsujan
Copy link
Member

tsujan commented Apr 11, 2025

The advantage: A much cleaner and readable patch that contains only the relevant changes. That's why the translation PRs are always made separately (by @stefonarch).

mActions.reset(nullptr);
mMenu->addAction(tr("power-profiles-daemon not available"))->setDisabled(true);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little uncomfortable about showing the new menu-item when power-profiles-daemon isn't available. This may pretend that power-profiles-daemon is preferred by LXQt, while many users may install tlp, which is in conflict with it.

IMHO, this deserves a discussion before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well we don't necessarily depend on power-profiles-daemon, but on provided DBus service. Should we rather use org.freedesktop.UPower.PowerProfiles DBus service not available?

Copy link

Choose a reason for hiding this comment

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

power-profiles-daemon is an actual package that the user can install to fix the problem, so it is much more user friendly imo.

Copy link
Member

@tsujan tsujan Apr 11, 2025

Choose a reason for hiding this comment

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

Should we rather use …

Too technical.

Why not hiding the menu-item instead?

EDIT: I mean the Power Profile menu-item, not just its submenu.

Copy link

Choose a reason for hiding this comment

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

I think that's a good solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not hiding the menu-item instead?

OK. Done

@tsujan
Copy link
Member

tsujan commented Apr 11, 2025

@libalis
Just a screenshot that shows my point about icon sets:

icon

@libalis
Copy link

libalis commented Apr 11, 2025

@libalis Just a screenshot that shows my point about icon sets:

icon

Very good illustration, thanks a lot!

Copy link
Member

@tsujan tsujan left a comment

Choose a reason for hiding this comment

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

GTM, as far as I'm concerned.

If you've tested it, please merge it — it'll be a nice feature in 2.2.0 — if not, @stefonarch will be available after April 15 and could test it.

@stefonarch
Copy link
Member

I'm not completely unavailable, for such a nice thing, will test it this evening but I'm confident. Translations have first to be committed in weblate, to avoid conflicts. I'll do all of them 15/16 April

@tsujan
Copy link
Member

tsujan commented Apr 11, 2025

We have time until April 17 — but not a day later ;)

@stefonarch
Copy link
Member

@palinek

Cloning into 'lxqt-powermanagement'...
done.
Switched to a new branch 'makepkg'
==> Starting pkgver()...
==> ERROR: pkgver is not allowed to contain colons, forward slashes, hyphens or whitespace.
==> ERROR: pkgver() generated an invalid version: power-profiles
...
...

$ git branch -M power-profiles power_profiles

No big issue but I never can build packages with your PRs directly ;)

@luis-pereira
Copy link
Member

The power profile menu will only be available in a system with a battery. Systems without a battery (desktops) can also benefit from power profiles.

Well, the all tray icon functionality becomes unavailable if the the system doesn't have a battery.

@palinek
Copy link
Contributor Author

palinek commented Apr 11, 2025

Systems without a battery (desktops) can also benefit from power profiles

I already made next PR for that #428

@palinek
Copy link
Contributor Author

palinek commented Apr 11, 2025

==> ERROR: pkgver is not allowed to contain colons, forward slashes, hyphens or whitespace.

No big issue but I never can build packages with your PRs directly ;)

Heh. As I'm using dash as word separator in branch names and your build doesn't like it as it wants to use the branch name in version string.

Copy link
Member

@stefonarch stefonarch left a comment

Choose a reason for hiding this comment

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

Works fine.

@palinek palinek merged commit bff102d into master Apr 11, 2025
@palinek palinek deleted the power-profiles branch April 11, 2025 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants