[New Designs] Implement add link flow #642

aanorbel · 2023-12-11T19:35:16Z

Fixes ooni/probe#2595

Proposed Changes

Add intent-filter to MainActivity so v2 events are handled by it.
Load descriptor from the backend and send to AddDescriptorActivity to be displayed.
- Ensure AddDescriptorActivity doesn't lose state on configuration change.

…d add a `ViewModel` for state management

…oard-glitch

…ndroid into issues/2591

…dle-kotlin-upgrade

…yling

bassosimone

For now, I mainly have change requests related to make the code more documented, clear, and easy to understand. Thank you for putting this branch together! 🙏

app/src/main/AndroidManifest.xml

app/src/main/java/org/openobservatory/ooniprobe/activity/MainActivity.java

bassosimone · 2023-12-19T11:42:52Z

app/src/main/java/org/openobservatory/ooniprobe/activity/MainActivity.java

-                        .build().show(getSupportFragmentManager(), null);
+        if (Intent.ACTION_VIEW.equals(intent.getAction())) {
+            Uri uri = intent.getData();
+            if (uri == null) return;


Would you mind not using this style where we collapse if and clause in a single line? I tend to prefer expanding it into N lines with { and } because it's more readable. (Kotlin has better facilities for being compact and they read easy, while I'd rather stick on scholastic Java for Java code, which risks being more tricky.)

bassosimone · 2023-12-19T11:43:38Z

app/src/main/java/org/openobservatory/ooniprobe/activity/MainActivity.java

+                    runId = Long.parseLong(uri.getPathSegments().get(0));
+                } else if ("run.test.ooni.org".equals(host)) {
+                    runId = Long.parseLong(uri.getPathSegments().get(1));
+                }


Please, add a comment here for the else case mentioning we handle it below

bassosimone · 2023-12-19T11:45:40Z

app/src/main/java/org/openobservatory/ooniprobe/activity/MainActivity.java

+                    runId = Long.parseLong(uri.getPathSegments().get(1));
+                }
+            } catch (Exception e) {
+                e.printStackTrace();


Please, add a comment mentioning we handle all errors below.

Also, do we want to document that this exception occurred using third-party services?

bassosimone · 2023-12-19T11:53:30Z

...rc/main/java/org/openobservatory/ooniprobe/activity/add_descriptor/AddDescriptorViewModel.kt

+    fun finishActivity() {
+        finishActivity.value = true
+    }
+}


I think we should configure the kotlin editor to add an empty line at EOF.

...nobservatory/ooniprobe/activity/add_descriptor/adapter/AddDescriptorExpandableListAdapter.kt

app/src/main/java/org/openobservatory/ooniprobe/common/LocaleUtils.java

app/src/main/java/org/openobservatory/ooniprobe/common/TaskExecutor.kt

bassosimone · 2023-12-19T11:59:15Z

engine/src/main/java/org/openobservatory/engine/PESession.java

+        String response = session.ooniRunFetch(ctx.ctx,id);
+        Log.d(PESession.class.getName(), response);
+        return new Gson().fromJson(response, OONIRunFetchResponse.class);
+    }


…escriptor/AddDescriptorActivity.kt Co-authored-by: Simone Basso <bassosimone@gmail.com>

…tils.java Co-authored-by: Simone Basso <bassosimone@gmail.com>

…escriptor/AddDescriptorActivity.kt Co-authored-by: Simone Basso <bassosimone@gmail.com>

app/src/main/java/org/openobservatory/ooniprobe/activity/MainActivity.java

…ctivity.java

bassosimone

We're getting there! 🙏

bassosimone · 2023-12-20T11:56:21Z

app/src/main/java/org/openobservatory/ooniprobe/common/LocaleUtils.java

@@ -9,8 +9,7 @@

 public class LocaleUtils {

-
-    private static Locale sLocale;
+    public static Locale sLocale;


Given how this class is structured, I would suggest adding a public getter here instead

engine/src/main/java/org/openobservatory/engine/OONIRunFetchResponse.kt

bassosimone · 2023-12-20T19:05:39Z

...src/main/java/org/openobservatory/ooniprobe/activity/add_descriptor/AddDescriptorActivity.kt

+                        else -> false
+                    }
+                }
+            binding.bottomBar.setOnMenuItemClickListener(bottomBarOnMenuItemClickListener)


Maybe split this long block of code by adding blank lines and brief comments?

bassosimone · 2023-12-20T19:07:21Z

app/src/main/java/org/openobservatory/ooniprobe/activity/MainActivity.java

+     * Get the run id from the intent.
+     * The run id can be in two different formats.
+     * <p>
+     * 1. ooni://runv2/<link_id>


If javadoc is intepreting HTML tags, maybe <link_id> will be interpreted, can we check?

bassosimone · 2023-12-20T19:09:24Z

app/src/main/java/org/openobservatory/ooniprobe/activity/MainActivity.java

+     * @param uri The intent data.
+     * @return The run id if the intent contains a link with a valid `link_id`.
+     */
+    private long getRunId(Uri uri) {


Can we return an optional rather than using a sentinel value here? I think this would be a bit more robust and would clarify a bit more the possibility that a value is out of range.

app/src/main/java/org/openobservatory/ooniprobe/activity/MainActivity.java

bassosimone · 2023-12-20T19:14:21Z

app/src/main/java/org/openobservatory/ooniprobe/activity/MainActivity.java

+            long runId = getRunId(uri);
+
+            // If the intent contains a link, but the `link_id` is zero, or the link is not supported, do nothing.
+            if (runId == 0) {


If you want to stick with returning an integer pattern, this would probably be:

Suggested change

if (runId == 0) {

if (runId <= 0) {

To exclude all the numbers that don't belong to the domain

bassosimone

LGTM with the understanding that you would apply the provided suggestions to avoid misleading the reader about the structure of OONI Run v2 links.

🙏 🐳

app/src/main/java/org/openobservatory/ooniprobe/activity/MainActivity.java

Co-authored-by: Simone Basso <bassosimone@gmail.com>

## Proposed Changes - Add supported icons in project and track.

…#653) Fixes # <issue> ## Proposed Changes - Adds uninstall button - Adds setting for auto-update - Updates preferences to properly save autorun settings

…erent screen (#657) ## Proposed Changes - Add `OoniRunV2Activity` to handle all incoming OONI Run v2 `Intents` - Update `AndroidManifest.xml` to launch `OoniRunV2Activity` when OONI Run v2 `Intents` are received. - Clean `MainActivity` - Override `onBackPressed` and `finish` in `AddDescriptorActivity` to ensure activity always launches `MainActivity` when closed |.| Possible Areas of Improvement | |-|-| | ![Screenshot_20240125_205311](https://github.com/ooni/probe-android/assets/17911892/595a9100-9fd6-4baf-b9d0-f0f024402fa7)| -Improve on error messages. <br/> - Merge with `AddDescriptorActivity` to make a single activity with 2 fragments for each of the views (requires a bit of core rewrite). | --------- Co-authored-by: Simone Basso <bassosimone@gmail.com>

…o issues/2595

aanorbel added 30 commits August 30, 2023 07:23

Replace HeterogeneousRecyclerAdapter with RecyclerView.Adapter an…

f1392aa

…d add a `ViewModel` for state management

Update annotation processing for tests

5ac4479

Merge branch 'master' of github.com:ooni/probe-android into fix/dashb…

4d0a80d

…oard-glitch

Updated shared module

631230a

AGP upgrade

1f813af

Updated roboelectric

9b41077

Merge branch 'master' of github.com:ooni/probe-android into fix/dashb…

059ef51

…oard-glitch

Upgrade gradle to 8.1.2 and add support for kotlin and kapt

66ffbc3

Update from ru.noties:markwon:2.0.1 to io.noties.markwon:core:4.6.2

09d2685

Change dependency naming

9353e64

Merge branch 'chore/gradle-kotlin-upgrade' of github.com:ooni/probe-a…

657a6c9

…ndroid into issues/2591

Updated OverviewActivity. and added ReadMorePlugin

6fd4edf

Update overview activity to have a collapsable toolbar

d532158

Updated views to match new design

9e4ea1d

Convert ProgressFragment to kotlin

9bbe78d

Updated text and icon for ProgressFragment

d1c53cd

Updated rules for displaying test complete notification

de8667b

Merge branch 'master' of github.com:ooni/probe-android into chore/gra…

1c9db05

…dle-kotlin-upgrade

Updated translations trings

c4d1e68

Start implementation of RunTests flow

9309eff

Updated button status

51f350e

Updated suite to run from BaseDescriptor

1461e29

Modified views and refactored code

1c5a84a

Modified preferences and activity actions

20a3160

Updated rules for storing preferences

5fbc024

Modified Menu and toolbar views for RunTestsActivity with proper st…

3902641

…yling

Updated the drawable tint for group icon indicator

2044002

Merge branch 'master' of github.com:ooni/probe-android into issues/2589

30d799b

Merge branch 'master' of github.com:ooni/probe-android into issues/2591

8c1ab25

revert changes on strings.xml

2d4437d

Update flow to include proper progress

44b4ac5

aanorbel requested review from bassosimone and hellais December 19, 2023 11:27

aanorbel marked this pull request as ready for review December 19, 2023 11:27

bassosimone suggested changes Dec 19, 2023

View reviewed changes

aanorbel and others added 5 commits December 20, 2023 08:05

Update app/src/main/java/org/openobservatory/ooniprobe/activity/add_d…

aa702c5

…escriptor/AddDescriptorActivity.kt Co-authored-by: Simone Basso <bassosimone@gmail.com>

Update app/src/main/java/org/openobservatory/ooniprobe/common/LocaleU…

86342f1

…tils.java Co-authored-by: Simone Basso <bassosimone@gmail.com>

Update app/src/main/java/org/openobservatory/ooniprobe/activity/add_d…

d3bd5d7

…escriptor/AddDescriptorActivity.kt Co-authored-by: Simone Basso <bassosimone@gmail.com>

Update app/src/main/java/org/openobservatory/ooniprobe/activity/add_d…

5a42afb

…escriptor/AddDescriptorActivity.kt Co-authored-by: Simone Basso <bassosimone@gmail.com>

Updated code based on review

44ea86d

aanorbel commented Dec 20, 2023

View reviewed changes

app/src/main/java/org/openobservatory/ooniprobe/activity/MainActivity.java Outdated Show resolved Hide resolved

aanorbel and others added 2 commits December 20, 2023 11:06

Update app/src/main/java/org/openobservatory/ooniprobe/activity/MainA…

dad3f90

…ctivity.java

Updated code based on review

788d22a

aanorbel requested a review from bassosimone December 20, 2023 10:28

bassosimone suggested changes Dec 20, 2023

View reviewed changes

aanorbel added 3 commits December 21, 2023 13:15

Updates based on review

b439010

Update code based on review

d5c8531

Merge branch 'master' of github.com:ooni/probe-android into issues/2595

db37b06

bassosimone approved these changes Dec 22, 2023

View reviewed changes

aanorbel and others added 7 commits December 22, 2023 11:38

Update based on review

ae929c8

Feat: Add database model persistence (#645)

8375440

Co-authored-by: Simone Basso <bassosimone@gmail.com>

Feat: Add Icon support for OONI Run v2 descriptors (#649)

5feeaab

## Proposed Changes - Add supported icons in project and track.

Feat: Update OverviewActivity.java interface with missing functions (…

bd68a91

…#653) Fixes # <issue> ## Proposed Changes - Adds uninstall button - Adds setting for auto-update - Updates preferences to properly save autorun settings

Merge branch 'dev/design-update' of github.com:ooni/probe-android int…

1b2ac66

…o issues/2595

Modified AbstractDescriptor definition

4bf6085

aanorbel changed the base branch from dev/design-update to dev/ooni-run-v2 February 16, 2024 09:52

aanorbel merged commit c459e1c into dev/ooni-run-v2 Feb 16, 2024
4 of 6 checks passed

aanorbel deleted the issues/2595 branch February 16, 2024 09:53

aanorbel mentioned this pull request Feb 16, 2024

OONI Run v2 #671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New Designs] Implement add link flow #642

[New Designs] Implement add link flow #642

aanorbel commented Dec 11, 2023 •

edited

Loading

bassosimone left a comment

bassosimone Dec 19, 2023

bassosimone Dec 19, 2023

bassosimone Dec 19, 2023

bassosimone Dec 19, 2023

bassosimone Dec 19, 2023

bassosimone left a comment

bassosimone Dec 20, 2023

bassosimone Dec 20, 2023

bassosimone Dec 20, 2023

bassosimone Dec 20, 2023

bassosimone Dec 20, 2023

bassosimone left a comment

[New Designs] Implement add link flow #642

[New Designs] Implement add link flow #642

Conversation

aanorbel commented Dec 11, 2023 • edited Loading

Proposed Changes

bassosimone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bassosimone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bassosimone left a comment

Choose a reason for hiding this comment

aanorbel commented Dec 11, 2023 •

edited

Loading