-
Notifications
You must be signed in to change notification settings - Fork 807
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
fix: state lost on orientation change in sensor activities #2576
fix: state lost on orientation change in sensor activities #2576
Conversation
Reviewer's Guide by SourceryThe changes refactor sensor activities to extend a new AbstractSensorActivity base class and improve state handling during orientation changes. The implementation moves common sensor control dock functionality to a shared layout and base class, while adding proper state saving/restoration. Class diagram for refactored sensor activitiesclassDiagram
class AbstractSensorActivity {
<<abstract>>
+SensorDataFetch sensorDataFetch
+getScienceLab() I2C
+onSaveInstanceState(Bundle)
+getSensorDataFetch() : SensorDataFetch
+getLayoutResId() : int
+getTitleResId() : int
}
class SensorMPU925X {
+SensorDataFetch sensorDataFetch
+MPU925x sensorMPU925X
+ArrayList<Entry> entriesAx
+ArrayList<Entry> entriesAy
+ArrayList<Entry> entriesAz
+ArrayList<Entry> entriesGx
+ArrayList<Entry> entriesGy
+ArrayList<Entry> entriesGz
+LineChart mChartAcceleration
+LineChart mChartGyroscope
+TextView tvSensorMPU925Xax
+TextView tvSensorMPU925Xay
+TextView tvSensorMPU925Xaz
+TextView tvSensorMPU925Xgx
+TextView tvSensorMPU925Xgy
+TextView tvSensorMPU925Xgz
+TextView tvSensorMPU925Xtemp
+Spinner spinnerSensorMPU925X1
+Spinner spinnerSensorMPU925X2
+Spinner spinnerSensorMPU925X3
+Spinner spinnerSensorMPU925X4
}
class SensorMPU6050 {
+SensorDataFetch sensorDataFetch
+MPU6050 sensorMPU6050
+ArrayList<Entry> entriesAx
+ArrayList<Entry> entriesAy
+ArrayList<Entry> entriesAz
+ArrayList<Entry> entriesGx
+ArrayList<Entry> entriesGy
+ArrayList<Entry> entriesGz
+LineChart mChartAcceleration
+LineChart mChartGyroscope
+TextView tvSensorMPU6050ax
+TextView tvSensorMPU6050ay
+TextView tvSensorMPU6050az
+TextView tvSensorMPU6050gx
+TextView tvSensorMPU6050gy
+TextView tvSensorMPU6050gz
+TextView tvSensorMPU6050temp
+Spinner spinnerSensorMPU60501
+Spinner spinnerSensorMPU60502
+Spinner spinnerSensorMPU60503
+Spinner spinnerSensorMPU60504
}
AbstractSensorActivity <|-- SensorMPU925X
AbstractSensorActivity <|-- SensorMPU6050
note for AbstractSensorActivity "Base class for sensor activities with state management"
note for SensorMPU925X "Refactored to extend AbstractSensorActivity"
note for SensorMPU6050 "Refactored to extend AbstractSensorActivity"
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Build successful. APKs to test: https://github.com/fossasia/pslab-android/actions/runs/12473976150/artifacts/2357569517 |
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @marcnause - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
c8639ec
to
89f41f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @marcnause - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Incorrect keys used for restoring Y and Z entries from saved state (link)
Overall Comments:
- Consider using SavedStateHandle from ViewModel instead of manual state restoration for a more modern approach to handling configuration changes
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 2 other issues
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sorry for this huge PR. One thing led to another. |
@marcnause This is fantastic! Sorry for the late review. I tested with the following three sensors:
I only had these three available with me so I could test with only three of them. |
4711495
to
4cce908
Compare
@AsCress, I tested with the following sensors tonight: VL53L0X, BMP180, APDS9960, CCS811 I found one bug and fixed it (df39058). The CCS811 and the APDS9960 need some time to output valid values after initialization. The sensors are initialized every time the orientation is changed. As a consequence the first few values are 0 and they overwrite the values which were stored and re-created on orientation change. The gap you noticed is also a consequence of the destruction and new setup of the thread which gets the data from the sensor. Both problems could be solved by moving the fetching of the data to a service which would keep running while the Activities are destroyed and re-created during orientation change. I think we should create an extra ticket for that. edit: Extra ticket created: #2583 |
0c00b9b
to
e1e84df
Compare
e1e84df
to
bd66db6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcnause I built the app from your branch today. It all goes as we discussed yesterday. I tested with four sensors: the ADS1115
, APDS9960
, VL53L0X
, BMP180
, all of them work fine now.
The only issue I noticed was a small misalignment of text in the sensor activity for the VL53L0X
. Attaching screenshots for your reference -
Rest, we are good to go.
How do you think we should go about this now ? Shall we wait and test with all the sensors ? Or should we just merge this ?
Good catch! Fixed in 786e6f5.
Since we both don't have all the sensors yet, I'd propose to merge it now and test all sensors later. I have created an issue to test all supported sensors: #2594 |
786e6f5
to
d63bccf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcnause Perfect then! We are good to go with this one.
Fixes #2528
Changes
Screenshots / Recordings
Checklist:
strings.xml
,dimens.xml
andcolors.xml
without hard coding any value.strings.xml
,dimens.xml
orcolors.xml
.Summary by Sourcery
Refactor sensor activities to extend a new AbstractSensorActivity base class, addressing state loss on orientation change and reducing code duplication. Introduce a new feature to save and restore the state of the sensor dock on orientation change. Enhance layout management by moving duplicate components to a new layout file and using tags.
New Features:
Bug Fixes:
Enhancements: