Feature/Ask for setup password, configure charge modes to display and add decorator for active charge mode

vsimkus / gnome-shell-extension-dell-command-configure-menu

A gnome-shell extension that adds submenu options access to Dell's Command Configure utility.

GNU General Public License v3.0

8 stars 1 forks source link

Feature/Ask for setup password, configure charge modes to display and add decorator for active charge mode #2

Closed raduburla closed 2 years ago

raduburla commented 2 years ago

hello,

In this quite big pull request I added the following:

an option to be prompted for Setup/Bios password (I personally have Setup password)
added Adaptive charge as a charge mode
added options to enable or disable preferred charge modes (My laptop does not have Express charge)
added decorator for active charge mode

This is my first time developing an gnome shell extension, hopefully the changes are ok, the docs are quite hard to find, mostly relied on existing extensions.

I tested the changes on Pop!_OS 21.10 with 40.4.0 GNOME version and Dell Command Configure Version 4.2.0

vsimkus commented 2 years ago

Hi, thanks for the PR! These are sensible changes and I'll try and review them soon when I find some time.

One thing I'll ask you to remove though - the active charge mode decorator. Whilst it is a feature I would like, the issue is that the charge mode can be changed in at least two more ways - the BIOS and the command line. This would cause the internal state of the extension indicate the wrong charge mode and hence confuse the users. Ideally, I would like to pull this state directly from cctk but that requires privileged rights and hence prompting the user for password, which I'd rather not do unless the user wants to change the charge state. So for now, let's remove this feature.

raduburla commented 2 years ago

Hi, I understand what you are saying and I was aware of it.

Some users might be Linux only and control everything from the OS and with an extension like this, only from the extension, me included :P How about leaving it as an extension option, maybe off by default? I can also add a small notice/warning so users can be aware that external changes will not propagate to the extension.

vsimkus commented 2 years ago

Sure, we let's add the feature disabled by default with a warning in the preferences menu.

vsimkus commented 2 years ago

Finally had some time to take a look. The feature is well-done and working great. I've fixed a couple of bugs, added the option to enable/disable charge mode indicator, and did some refactoring on top. Now it is ready to merge. Thanks for the PR!

raduburla commented 2 years ago

I'm glad you could make time. Was planing to make some time myself today to add the option for charge mode indicators :) Thank you!