-
-
Notifications
You must be signed in to change notification settings - Fork 281
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
Code refactoring and add some features #712
Conversation
This PR is simply too large and all over the place to be reviewed in any meaningful manner. A lot of this stuff to me seems like you're just making changes for the sake of change. Instead of focusing on this stuff, I think you should look at this list of issues and requests that Adnan put together We want everyone who wants to contribute to be able to contribute, but this comes with certain standards that need to be kept. In this case, Adnan and I (or anyone for that matter) do not have time nor want to review a PR that touches 40 different files. These are just my thoughts. I'll let @AdnanHodzic decide what to do with this. |
I agree with what @shadeyg56 said. It's a better idea to split this into i.e 5 PR's. One for battery, one for installer and so on. Of course refering to the mentioned list would be ultimate goal. I think #29 (or another similiar issue) is part of installer changes but not sure as this is not mentioned anywhere. |
I understand what you're saying, except when you say "you're just making changes for the sake of change" But in this PR, I'm changing almost nothing about the way auto-cpufreq works, it's mainly a code overhaul to make it more readable The only change that could affect the way it works is about the battery , instead of using So I'm going to complete my message explaining my changes and try to find some testers to make sure everything works properly |
Adnan hodzic master
Since as @shadeyg56 pointed out, there's too many changes made for 1 PR which are hard to review, so I just jumped into testing things. Please note, just to remove any ambiguity. I appreciate what you're doing and I want to you and your changes to be part of the projects, so please don't take this the wrong way but certain procedures and standards will have to be followed. From brief view on what's going on, I would've rejected this PR. This is not a code refactor, this PR breaks design/way auto-cpufreq works which should never be the case of "code refactors". As I think you made some major changes without really understanding what's happening or why certain changes are there, which cannot be considered as a code refactor. Considering what I've just seen, this seriously erodes my my trust into rest of changes I didn't even get to check. But instead of rejecting it and closing the PR, you'll need to address following points before we can even start working on reviewing the code. Namely: Output of
First there's few too many lines which I would get rid of, i.e:
Above Same is when uninstalling part i,e:
Why are there 2 empty "Info:" lines? Second is a major breaking change which I won't merge as it reverts months/years of work that was done since 1.8.0 release.
Power profiles daemon will interfere and why auto-cpufreq disables it as part of auto-cpufreq-installer, this is explained here as part of Readme and as of v1.8.0 auto-cpufreq marks this functionality redundant.
We don't even suggest it, code will disable it for you if it detects it's running. So why are you enabling it back? This was a suggestion made to folks who installed auto-cpufreq using snap package, as due to it running in confined space with limited permissions and as such auto-cpufreq is running in a container it doesn't have as many permissions to disable Power Profile daemon like auto-cpufreq-installer can. That's why users were suggested to disable it manually using Third is the same subject I mentioned in Second point, it should be disabled by default, I'm fine with giving users option to enable it back if they know what they are doing, which they usually don't.
Again, why not just split this PR into few smaller PR's so code can be reviewed easier and changes tested easier? For example PR for "installer", PR for "Init system", PR for "config" as I don't think I'll ever merge it as one such big PR, there's just too many changes and a lot of them don't make sense. For example just changes for installer part:
I think you mean "changes" instead of "Changements". If you could also elaborate more on certain changes like:
Why did you add these options?
Why, as of #663 changes in conf file are automatically picked up?
Why did you make this decision? What's its benefit? |
Separate changes in different PR |
Batteries
Make battery detection and optimisation compatible with more machines
Changements
find
command and look for the/sys/class/power_supply/BAT*
folders to detect all batteriescore.py
)/sys/bus/platform/drivers/ideapad_acpi/VPC2004:00/conservation_mode
with all the files found byfind /sys/devices/ -type f -name conservation_mode
echo value | tee battery_file
, it bypasses the symbolic link problem [ThinkBook] FileNotFoundError: [Errno 2] No such file or directory: '/sys/class/power_supply/BAT0/charge_start_threshold' #7136 files changed
auto_cpufreq/batteries.py
createdauto_cpufreq/core.py
modifiedauto_cpufreq/battery_scripts/
removed (4 files removed)Config
Changements
$XDG_CONFIG_HOME/auto-cpufreq.conf
detectionfind_config_file
andconfig_info_dialog
withconfig.set_file
8 files changed
README.md
,auto_cpufreq/bin/auto_cpufreq.py
andauto_cpufreq/core.py
modifiedauto_cpufreq/utils/
moved toauto_cpufreq/config/
config.py
,event_handler.py
modifiedauto-cpufreq.conf-example
renamed toauto-cpufreq.conf
nix/module.nix
modifiedDistro detection
Make distro detection compatible with more system
Changements
distro
packagegetoutput('cat /etc/os-release')
, it bypasses the symbolic link problem4 files changed
auto_cpufreq/core.py
,nix/default.nix
,poetry.lock
, andpyproject.toml
modifiedInit system
Make daemon installation and
power_helpers.py
compatible with more systemChangements
5 files changed
auto_cpufreq/init_system.py
createdauto_cpufreq/power_helper.py
,scripts/auto-cpufreq-install.sh
,scripts/auto-cpufreq-remove.sh
andscripts/auto-cpufreq-dinit
modifiedInstaller
Changements
-i
and-r
options/etc/auto-cpufreq.conf
python3
to avoid error for "python command does not exist"1 file changed
auto-cpufreq-installer
modifiedUpdate
Changements
/tmp/
core.py
)4 files changed
auto_cpufreq/update.py
createdauto_cpufreq/bin/auto_cpufreq.py
,auto_cpufreq/core.py
andauto_cpufreq/gui/app.py
modifiedOthers
Changements
-m
=--monitor
,-l
=--live
,-i
=--install
,-u
=--update
,-r
=--remove
and-f
=--force
)'/etc/bluetooth/main.conf'
exists, fixes the problem for those who havebluez
but notbluez-utils
/bluetoothctl
--log
optionFiles changed
auto_cpufreq/commands.py
,auto_cpufreq/dialogs.py
,auto_cpufreq/globals.py
andauto_cpufreq/prints.py
createdauto_cpufreq/bin/auto_cpufreq.py
,auto_cpufreq/bin/auto_cpufreq_gtk.py
,auto_cpufreq/core.py
,auto_cpufreq/gui/app.py
,auto_cpufreq/gui/objects.py
,auto_cpufreq/gui/tray.py
,scripts/auto-cpufreq-venv-wrapper
andauto_cpufreq/power_helper.py
modified__init__.py
removed (4 files removed)auto_cpufreq/tlp_stat_parser.py
removedscripts/start_app
renamed toscripts/start_app.sh