Add password-less changes support, localisation support and other changes

AzzamAlsharafi / ideapad-controls-gnome-extension

GNOME Shell extension for controling Lenovo IdeaPad laptops options.

GNU General Public License v3.0

27 stars 5 forks source link

Add password-less changes support, localisation support and other changes #12

Closed Woomymy closed 11 months ago

Woomymy commented 11 months ago

This PR is focused on 3 features:

Allow setting up permissions and disabling pkexec wrapper to write ideapad_laptop sysfs nodes (#7)
Add localisation support (#11) (currently, only french and english are supported)
Add success notifications when settings are changed (configurable)

Some other (mostly dev-related) changes were made:

A .gitignore file was added to prevent pushing unwanted files on GitHub
template.ui was ported to Gtk blueprint (and is converted to xml when building, blueprint-compiler is a submodule and compilation is done by make on install)
make install replaces the install.sh script
There's an option to change the /sys path of thee ideapad_acpi device, which shouldn't be used for now, but can be helpful in the future.

Tell me if you have any suggestion / request of enhancement I can make to this PR (if you want to know more about the modifications, look at the commit descriptions)

Thanks for your work on this extension, Woomy.

Woomymy commented 11 months ago

Goes without saying that all the changes where tested and shouldn't cause any problems, but some supplementary testing wouldn't hurt (it works... on my machine :))

AzzamAlsharafi commented 11 months ago

I'm currently in a trip for some days so I won't be able to check it for a while, but I would like to thank you for this PR.

I never expected anyone to contribute to this repo, so this is greatly appreciated.

Woomymy commented 11 months ago

No problem, enjoy your trip !

AzzamAlsharafi commented 11 months ago

Hello, I just reviewed the changes.

They are wonderful, but I just have few minor notes:

For the sysfs entry, I'm worried that users might edit it by mistake and then get stuck, because they don't know what is the original correct value. I know this mistake can be fixed easily, but I want the extension to be as user-friendly as possible, so can you for example add a button to reset the field to the default value.
The success notifications are only sent when pkexec is disabled. Is this intentional? If so then I think it should be clarified to the user, for example you could disable the notifications switch when pkexec is enabled.
The README file should clarify that both make and gettext need to be installed for the make install command to work.

Other than that, everything else is great, and I really appreciate your help for improving this extension. Thank you very much.

Woomymy commented 11 months ago

The three issues should be addressed now, thanks

AzzamAlsharafi commented 11 months ago

All right, just final change, can you add valign: center; to the reset button.

Thanks.

Woomymy commented 11 months ago

Looks a lot better now, thanks !

AzzamAlsharafi commented 11 months ago

Ok, so I uploaded the updated extension to extensions.gnome.org, and it was rejected, with the following message:

You cannot create instances in global scope which is the same as init:

line 6 optionsUtils.js

line 14 prefs.js

https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization

Too much for global scope (line 14 optionsUtils.js): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization

and another thing, I noticed that when pkexec mode is enabled, the extension sends a success notification even before the user enters the password, which doesn't make sense from a user perspective, so is it possible to make it so that notifications are only sent after the user enters the correct password? I tried to do that before but I wasn't able to find a way to do it, so do you know how something like that can be done?

If not possible then I think it's better to just disable notifications whenever pkexec is enabled like before, and make it so that the settings page disables the notifications switch when pkexec is enabled.

Can you handle these two changes (rejection, and notifications)? I know I've already asked too much, so if you don't want it's ok. Hopefully these will be the last changes to this PR.

Thanks.

Woomymy commented 11 months ago

Sorry, took me some time to remember I had to fix it, does it look fine now?

AzzamAlsharafi commented 11 months ago

Looks good to me. I'll send it for review at gnome.extensions now.

Woomymy commented 11 months ago

Thanks !

AzzamAlsharafi commented 11 months ago

Rejected with this comment:

Also null out extensionSettings on destroy (line 26 optionsUtils.js).

I assume this should be the final issue, so I'll merge this PR and then I'll fix the issue.

Thanks for your great work! You implemented all the stuff I wasn't able to from the Issues and more.

Woomymy commented 11 months ago

Nice, thanks !

ViBE-HU commented 11 months ago

In some languages the conjugation looks ugly or not understandable if there is no separate strings for different events. Could you please add stand-alone event messages for the notification messages?