Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add SettingsRepo, refactoring & setting for battery limit #28

Merged
merged 34 commits into from
May 15, 2024
Merged

Add SettingsRepo, refactoring & setting for battery limit #28

merged 34 commits into from
May 15, 2024

Conversation

miguelguzmanr
Copy link
Collaborator

Things to note:

  • Still testing these changes. My sincerest apologies if anything broke.
  • Could not decide on a name or correct folder for the SettingsRepo class. Please change.
  • Could not decide on a proper name for functions that use "cat"/"echo" commands in the ShellExecutor class to read and write settings. Not sure if they're even useful at all, either, at least currently not very much. Please change.

@miguelguzmanr
Standardize formatting and color values of icons
9cdc836 
- Apply consistent indentation
- Use android constants for color values instead of hex values
@miguelguzmanr
Merge remote-tracking branch 'upstream/main' into charging-separation
2ca10a8
@miguelguzmanr
Rename face buttons
8474896 
Attempt to use generic icon names to promote re-usable icons
@miguelguzmanr
Allow title in CheckBoxDialogPreference
36f4b41
@miguelguzmanr
Attempt consistency in titles & descriptions
890cf4f 
Use consistent capitalization and punctuation.
@miguelguzmanr
Add SettingsRepo & refactoring
b8e7196 
- Add a class for system settings.
- Attempt consistency in function & variable names.
@miguelguzmanr
Add padding in RemapButtonDialog
dec5050
@miguelguzmanr
Add setting to bypass charge after 80%
390b5c6
@miguelguzmanr
Merge pull request #1 from miguelguzmanr/charging-separation
e048cb7 
Add SettingsRepo, refactoring & setting for battery limit
langerhans
langerhans requested changes Feb 15, 2024
View reviewed changes
Copy link
Owner

@langerhans langerhans left a comment

Choose a reason for hiding this comment

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

For the name I think it's fine as it is. I'm bad at naming things so I don't worry so much about it. As for the package maybe we can move SettingsRepo and ShellExecutor to a new package "system" since they are responsible for communicating with the system. Keep DeviceUtils in tools and BootReceiver can live there for now as well. Maybe that will be a bit cleaner.
Otherwise nice work, thanks!

app/src/main/java/de/langerhans/odintools/tools/DeviceUtils.kt Outdated Show resolved Hide resolved
app/src/main/java/de/langerhans/odintools/tools/SettingsRepo.kt Outdated Show resolved Hide resolved
app/src/main/java/de/langerhans/odintools/tools/SettingsRepo.kt Outdated Show resolved Hide resolved
app/src/main/java/de/langerhans/odintools/tools/SettingsRepo.kt Outdated Show resolved Hide resolved
app/src/main/java/de/langerhans/odintools/tools/SettingsRepo.kt Outdated Show resolved Hide resolved
@miguelguzmanr
Copy link
Collaborator Author

I suggest we put these changes on hold while we review other pull requests @langerhans

@miguelguzmanr
Copy link
Collaborator Author

miguelguzmanr commented Apr 21, 2024 •
edited
Loading

Introduces configurable charging separation.
Requires thorough testing, as this may introduce additional battery drain due to having to monitor the battery level.

image

@miguelguzmanr
Copy link
Collaborator Author

This approach only works if the app is running in the background. I think this might need to be implemented as a foreground service or a background task.

@langerhans
Copy link
Owner

Even with the fix from #23 this does not work in background? Could we just watch battery state with Intent.ACTION_BATTERY_CHANGED?

@miguelguzmanr
Copy link
Collaborator Author

miguelguzmanr commented Apr 28, 2024 •
edited
Loading

Introduces a service to monitor the battery level.
Just trying out a different approach.
Sadly, the service forcefully ends when the app is terminated in any way.

Requires #23 to work in the background.

image

Fixed incorrectly reading restrictCharge setting as a system setting instead of as a text file.
Discovered that charging separation briefly turns off when the screen is turned off. Not sure about the culprit. Added a trigger for screen off event to combat this behavior.
Should work properly now.

Added an option to dump a log file for debug builds.
Might be useful for troubleshooting issues.

image

@miguelguzmanr
Copy link
Collaborator Author

@langerhans please let me know your thoughts!

langerhans
langerhans approved these changes Apr 28, 2024
View reviewed changes
Copy link
Owner

@langerhans langerhans left a comment

Choose a reason for hiding this comment

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

Happy to merge this, but just had two more questions :)

app/src/main/java/de/langerhans/odintools/main/MainActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/de/langerhans/odintools/service/BatteryLevelMonitorService.kt Outdated Show resolved Hide resolved
@github-staff github-staff deleted a comment from isedwards May 1, 2024
@miguelguzmanr
Copy link
Collaborator Author

@langerhans please let me know if this approach works better!

@langerhans
Copy link
Owner

Yup looks good to me, LGTM

@langerhans langerhans merged commit b32e54f into langerhans:main May 15, 2024
3 checks passed
@miguelguzmanr miguelguzmanr mentioned this pull request May 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@langerhans langerhans langerhans approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@miguelguzmanr @langerhans