-
Notifications
You must be signed in to change notification settings - Fork 47
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
[New Designs] Update Run button from dashboard to display new flow for selecting desired tests #632
Conversation
…dle-kotlin-upgrade
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.
Overall, it looks good to me. I had to ask some questions about some language features. Additionally, it seems to me we can remove a bunch of commented out lines and we should add some JavaDoc-like documentation.
app/src/main/java/org/openobservatory/ooniprobe/activity/runtests/RunTestsActivity.kt
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/activity/runtests/RunTestsActivity.kt
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/activity/runtests/RunTestsActivity.kt
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/activity/runtests/RunTestsActivity.kt
Outdated
Show resolved
Hide resolved
|
||
|
||
private fun updateStatusIndicator() { | ||
//TODO(aanorbel): translate status indicator |
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.
Do we want to do this in the same PR or in a subsequent one?
app/src/main/java/org/openobservatory/ooniprobe/test/suite/InstantMessagingSuite.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/test/suite/MiddleBoxesSuite.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/test/suite/PerformanceSuite.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/openobservatory/ooniprobe/test/test/AbstractTest.java
Outdated
Show resolved
Hide resolved
…nceManagerExtension.kt Co-authored-by: Simone Basso <[email protected]>
String tn, | ||
@Nullable List<String> urls, | ||
String origin) { | ||
for (AbstractSuite suite : TestAsyncTask.getSuites()) |
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.
Please, always use {
and }
even if there is just a single statement afterwards.
for (AbstractSuite suite : TestAsyncTask.getSuites()) | ||
for (AbstractTest test : suite.getTestList(app.getPreferenceManager())) | ||
if (test.getName().equals(tn)) { | ||
if (urls != null) |
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.
Likewise, {
and }
🙏
import org.openobservatory.engine.BaseDescriptor | ||
import org.openobservatory.engine.BaseNettest | ||
|
||
class ChildItem( |
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.
maybe name this ChildGroupItem
?
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.
Once all floating (and minor!) comments have been addresses, we can merge!
🐳
Fixes ooni/probe#2589
build: https://github.com/ooni/probe-android/suites/18778645278/artifacts/1094292643
Proposed Changes
RunTestsActivity
and related components