Skip to content
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

Feat: Add News Media Scan flavor #549

Merged
merged 54 commits into from
Jul 15, 2024
Merged

Feat: Add News Media Scan flavor #549

merged 54 commits into from
Jul 15, 2024

Conversation

aanorbel
Copy link
Member

@aanorbel aanorbel commented Dec 12, 2022

Proposed Changes

  • Update application based on designs
Light Dark
01_onboarding_2 .
01_onboarding_2 .
02_onboarding_2_q1 .
02_onboarding_2_q2 .
03_automated_testing .
04_crash_report .
05_default_settings .
06_dashboard 06_dashboard
07_test_results 07_test_results
08_test_results_dash 08_test_results_dash
08_test_results_webconnectivity 08_test_results_webconnectivity
09_settings 09_settings
10_info 10_info

Tasks

  • Test card coloring.
  • Onboarding colours.
  • Grainy images.
  • About page.

Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with merging this code into the master branch already under the argument that it's easier to keep things in sync if we have this code in tree.

In this review, I have tried to think about everything that we should clearly define for us to keep this app in sync with out mainline app. So, I think the focus of the discussion should not be much the code you're adding here, which is fine, but rather how we're planning on maintaining it. We can have this discussion as part of merging or we can create an issue describing the aspects we need to discuss in greater detail, depending on what's more convenient to you.

🐳

app/build.gradle Outdated
applicationId 'com.dw.ooniprobe'
resValue "string", "APP_NAME", "DW Media Probe"
buildConfigField 'String', 'BASE_SOFTWARE_NAME', '"dw-media-probe-android"'
versionName '0.0.1'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of maintainability, we should figure out before hand a way to map the dw-flavoured-app version to the actual ooniprobe-android version. The simplest scheme that comes to mind is that of using the same version number and the same version code we use for ooniprobe-android. Because using the same version number may be odd (would it be odd?), perhaps we can just share the version code? What do you think?

@@ -0,0 +1,43 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test whether it's possible to install the dw-flavoured-app and ooniprobe side by side? As far as I know, it is currently not possible to install OONI Probe stable and OONI Probe experimental side by side. While that may be acceptable because it's only us who install the experimental app, not allowing installation side by side will be a bit more confusing for people. That said, also allowing installing side-by-side is weird because we are going to have tow ~ooniprobe apps competing for collecting measurements. Did you spend time thinking about which would be the best way to handle this side by side scenario?

@@ -0,0 +1,404 @@
<PreferenceScreen xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that this file completely duplicates the ooniprobe preferences. I think we should start thinking about a way to be sure we always sync up preferences changes. Did you already start to think about this? What do you think is the best strategy to ensure this happens?

@@ -0,0 +1,30 @@
<vector xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Contributor

@bassosimone bassosimone Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this logo is there because we want to say something like "powered by", right?

engine/build.gradle Outdated Show resolved Hide resolved
Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐳

@hellais
Copy link
Member

hellais commented Mar 13, 2023

Issues reported on slack:

  1. The app crashed twice. Once when I first opened it and went to settings. The second time when the test finished and I clicked on the results
  2. The eta doesn't seem to ever appear. For the full duration of the test it always says calculating ETA
  3. The app cards for the disabled tests should not be clickable. Maybe we could have them go to the settings to enable them if we wish to
  4. When I go into the settings and enable one of the tests for the groups. It’s then not possible to re-disable them as when I click on them I get an error saying “Please enable at least one test”

@aanorbel
Copy link
Member Author

aanorbel commented Mar 16, 2023

  • The app crashed twice. Once when I first opened it and went to settings. The second time when the test finished and I clicked on the results
  • The eta doesn't seem to ever appear. For the full duration of the test it always says calculating ETA
  • The app cards for the disabled tests should not be clickable. Maybe we could have them go to the settings to enable them if we wish to Prevent click of disabled cards #562
  • When I go into the settings and enable one of the tests for the groups. It’s then not possible to re-disable them as when I click on them I get an error saying “Please enable at least one test” Allow all tests under category to be disabled #561

@aanorbel aanorbel requested a review from bassosimone December 5, 2023 16:03
@aanorbel aanorbel changed the title Initial prototype for DW Media Probe Feat: Add News Media Scan flavor Dec 7, 2023
Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐳

@aanorbel aanorbel merged commit 3106d03 into master Jul 15, 2024
13 of 14 checks passed
@aanorbel aanorbel deleted the dw-media-probe branch July 15, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants