buhtz
commented
3 weeks ago Hello Fanis, On behalf of the team, thank you for your contribution and taking the time to improve Back In Time. We appreciate it.
As I remember your contribution is part of something like a school home work, isn't it? Do you have a deadline or something?
Your PR is of good quality on a first look. But the behavior/feature is complex so will need some time to test and improve the PR:
Without reviewing it in detail yet please let me add some first impressions and questions.
- You are working directly on
devbranch. Leave it as it is now but next time please create your own branch based ondev. See CONTRIBUTING.md for detailed instructions. - I am not sure but it looks like that your
devbranch is outdated. Please update it. (e.g. use "Sync fork" button on your GitHub page) - Which one of the ACPI States is used here? Suspend to RAM (S3), Suspend to Disk (S4) or something else? Or is it more a "generalized" energy saving state and the OS settings define which one is used?
- Please name the source (document, website, ...) where you get the dbus settings (
DBUS_SLEEP) from? - Did you used any automation tool for code generation ("AI" stuff like CoPilot) or manipulation (
blackor other auto formatters)? - Please try to follow PEP8 coding guidelines
- e.g. maximum line length 80 chars.
- no camel case for method or variable names
- ...
- I would suggest to remove your code from
tools.pyand create your owncommon/suspend.py
EDIT: I don't know much about DBus and also doesn't have much experience with it. Do you have? The fixed dictionary DBUS_SLEEP doesn't feel to me as an elegant solution. Might it better to use Introspection to determine the the correct DBus Service/Name/Object/Method (e.g. StackOverflow question). To me it is also not clear if that names are different between the GNU Linux distributions and their desktop environments. It sounds complex but possible. I suggest to make an extra PR for the determine-suspend-mode-trigger-on-DBus-feature and put this current PR on hold.
Best, Christian
1268
I tried to create sleep mode for the Back in Time application. I also added a button on the GUI of the app. This is my first contribution so I hope I did well. Any feedback would be helpful.