Automatic dark mode based on ambient light #6756
Conversation
| private Activity activity; | ||
|
|
||
| public AutoDarkModeManager(Activity activity) { | ||
| this.activity = activity; |
There was a problem hiding this comment.
think this is a memory leak waiting to happen
| } | ||
|
|
||
| public void listenForCurrentActivityIfNecessary() { | ||
| if (!activity.getClass().getSimpleName().equals(lastActivityName)) { |
There was a problem hiding this comment.
this feels a little gross on two levels, first using the textual representation of a class name to track activity rather than identity, and then having potentially multiple instances of this class but only one listening
| showDarkTheme = lux <= MAX_LUX_FOR_DARK_THEME; | ||
| if (previousShowDarkTheme != showDarkTheme) | ||
| ActivityUtil.recreateActivity(activity); | ||
| stopListening(); |
| @@ -0,0 +1,51 @@ | |||
| /* | |||
| * Copyright (C) 2017 Fernando Garcia Alvarez | |||
There was a problem hiding this comment.
This would need to be copyright OWS with GPLv3 headers for me to merge
|
I have fixed the problems you mentioned. Please let me now of any problems. |
| Activity currentActivity = activity.get(); | ||
| if (previousShowDarkTheme != showDarkTheme && currentActivity != null) | ||
| ActivityUtil.recreateActivity(currentActivity); | ||
| stopListening(); |
There was a problem hiding this comment.
It looks like this is the only place you ever unregister a listener. It looks like that means that every time you enter an activity, you get a new instance of this class and a new listener. They stack up forever, unless there's a sensor event.
On the other hand, as soon as a sensor change comes in, whether it's below the threshold or not, it gets unregistered. From what I can understand, this behavior is both a memory leak and breaks the functionality?
|
|
||
| } | ||
|
|
||
| private synchronized void parseAmbientLightValue(SensorEvent event) { |
There was a problem hiding this comment.
Why is this method synchronized? If the idea is that it can be called off the UI thread, then it seems like all the values that it accesses would also need synchronized access from their writes on the UI thread.
|
|
||
| @Override | ||
| public void onSensorChanged(SensorEvent event) { | ||
| parseAmbientLightValue(event); |
There was a problem hiding this comment.
Private methods might read instance state if necessary, but they should not have the side effect of modifying instance state unless they're initializer methods. The method you're calling is named "parseAmbientLightValue," but it does much more than parsing an argument -- it modifies instance state, updates listeners, even potentially destroys an activity.
|
I have refactored AutoDarkModeManager to be cleaner, and also now the sensor gets unregistered as soon as onPause is called. The behavior of unregistering the sensor after the first value is found is correct, I did that in order to preserve battery life and also to avoid annoying the user, since for example if the user is typing a message and then it becomes necessary to switch to the dark theme based on ambient light, the user will get annoyed due to the activity being restart while typing the message, but if you want another behavior, let me know. |
|
I've also made a rebase to the pull request to update it to the new changes of Signal (and test it with the new 4.8.0 version) |
|
This PR currently includes some commits from master. Seems like something went wrong during rebase. Fixes #751. |
fercarcedo
force-pushed
the
autodarkmodeambient
branch
from
42a310e to
1678038
Compare
|
I have removed the merge commit from the PR, which was causing some master commits to appear here. Sorry, that's my first time doing a PR |
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
// FREEBIE
First time contributor checklist
Contributor checklist
Fixes #1234syntaxFREEBIEin the commit message of my first commitDescription
This pull request enables Signal to automatically change to dark theme based on the current ambient light condition, like many other apps do.
I have tested this pull request in both emulators (setting manually the number of lux), as well as on my Nexus 5, going from outdoors to a dark room indoors, and in the night outside.