-
Notifications
You must be signed in to change notification settings - Fork 804
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: disable channel range select when in running state #2551
fix: disable channel range select when in running state #2551
Conversation
Reviewer's Guide by SourceryThis pull request implements a fix to disable the channel range select dropdowns for CH1 and CH2 when the oscilloscope is in the running state. The changes primarily affect the OscilloscopeActivity class, introducing new functionality to manage the enabled state of the spinners based on the oscilloscope's running state. File-Level Changes
Tips
|
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 @Dishebh - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider initializing the spinners (spinnerRangeCh1 and spinnerRangeCh2) once in the onCreate() method and storing them as class members, rather than finding them by ID every time setChannelRangeEnabledState() is called. This would be slightly more efficient.
Here's what I looked at during the review
- 🟡 General issues: 2 issues 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 to tell me if it was helpful.
Build successful. APKs to test: https://github.com/fossasia/pslab-android/actions/runs/10966746843/artifacts/1960336218 |
hey @marcnause @AsCress , sorry for tagging you again, but if you get a chance to look at this PR, and find any issues, please let me know. Thanks! |
@sourcery-ai review |
f9855d9
to
dc2bc5a
Compare
The changes have the described effect. I am just not sure anymore if disabling channel range select is the solution of if it only hides a bug that should be fixed. Please let me investigate and come back to this PR once I have made up my mind. |
Thanks for your work on this issue ! |
@AsCress I checked the One additional problem I noticed: If CH1 and CH2 are selected, both are scaled according to the range set for CH1. The range for CH2 is ignored. But maybe we should handle this problem in an additional issue. |
I think there will be a problem if the lines are removed and a user switches to FFT mode and back. I think the whole issue is more complicated than I thought when I wrote my first comment in #2099. I think it would probably make things easier if all variables and methods that are connected to range selection is moved to an extra class. I will try to do some refactoring tonight if I find the time. |
@marcnause Hmm, I see. I think there may also be a problem with the Auto-Scale. Do inform me if you need any help with this (currently I am working a lot on the ESP and the Logic Analyzer, so didn't get time to push any changes to this PR or any other 😢). |
I have added a new pull request in which I try to fix the underlying problem: #2552 |
Hey, sure. Let me close this, if Marc's PR already fixes the issue. Thanks, @marcnause, for fixing this, and sorry I couldn't do it 😅 |
@Dishebh Thank you very much for taking on this problem. Your contribution was the first step towards solving this problem, without it the bug ticket would probably still be open! |
Fixes #2099
Changes
Screenshots / Recordings
Checklist:
strings.xml
,dimens.xml
andcolors.xml
without hard coding any value.strings.xml
,dimens.xml
orcolors.xml
.Summary by Sourcery
Fix the issue where the channel range select dropdown was active during the running state of the oscilloscope by disabling it, and refactor dataEntries to use a more consistent type.
Bug Fixes:
Enhancements: