-
Notifications
You must be signed in to change notification settings - Fork 46
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
Display total "Time remaining counter" instead of single timer for single test #487
Conversation
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.
My review could be broadly split into two parts:
-
I have asked for changes that overall seek to improve naming and documentation;
-
I have highlighted a bunch of places where I think we could possibly refactor and make the code a bit more abstract. I think they make sense but my limited Android skills could be failing me and I may be actually providing suggestions that make increase rather than decrease the complexity of the code. In which case, please, let me know and I'll keep in mind the lessons learned today for future reviews.
Thank you for preparing this diff!
app/src/main/java/org/openobservatory/ooniprobe/activity/RunningActivity.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/fragment/ProgressFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/fragment/ProgressFragment.java
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/fragment/ProgressFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/fragment/ProgressFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/receiver/TestRunBroadRequestReceiver.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/activity/RunningActivity.java
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/activity/RunningActivity.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/activity/RunningActivity.java
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/fragment/ProgressFragment.java
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/activity/RunningActivity.java
Outdated
Show resolved
Hide resolved
Hi @hellais , I will check the coloring for the progress bar. |
Co-authored-by: Simone Basso <[email protected]>
Hello @hellais , this problem comes specifically from ExperimentalSuite() and anim/experimental.json which is not theme aware. I would have to make various changes at a later time to make the resource change with theme. It is worth noting that this problem exists in all previous versions of the app. |
Ok I documented the animation background color issue here: ooni/probe#2038. For the rest we can proceed with merging this! |
Sure |
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.
🐳
* abstracted progress max to task * modified TestRunBroadRequestReceiver to publish progress to livedata component
Fixes ooni/probe#1936
Proposed Changes
TestRunBroadRequestReceiver.runtime
to summation ofTestAsyncTask.testSuites
runtime.progress
max value to equalpreviousTestsRuntime + currentTestProgress
timeLeft
from new values available.