Handle openfoodfacts.org links (Add deeplinking towards the product page)

teolemon commented 2 years ago

Who for

Potential users
Current users

What

We have many product pages on the web
There are in-browser mechanisms to propose installing the native app for those who visit the page in a browser, or directly from Search Engines
Allow Smoothie to handle openfoodfacts.org links

Design doc with links to implementations in the classic apps

Why

Convenient for existing users
Chrome might propose app install as a result after a few visits.
Google Search might propose app install in some conditions.

Part of

2515

Mockup

Code pointer

https://docs.flutter.dev/development/ui/navigation/deep-linking

Status

At the moment we handle these urls:

[x] Android
- [x] /product
- [x] /additive
- [x] /allergen
- [x] /category
- [x] /cgi/search.pl
- [x] /contributor
- [x] /country
- [x] /label
- [x] /manufacturing-place
- [x] /origin
- [x] /packaging
- [x] /state
- [x] /store
[ ] iOS:
- [ ] ...

Note that we should support those URLs in plural (list of all existing additives), and in singular (list of products for a given additive)

Urls we'd like to support

Depends on

[x] https://github.com/openfoodfacts/smooth-app/issues/260

M123-dev commented 2 years ago

Note: Its not straight forward, we have to switch to either named routes or router 2.0

260

g123k commented 2 years ago

You're right @M123-dev, before handling deep links, we have to migrate the app named routes or Navigator 2. I create a separate issue for migration.

g123k commented 2 years ago

@teolemon Could you make the document publicly readable, please?

teolemon commented 2 years ago

Fixed @g123k

g123k commented 2 years ago

On Android, here are all the supported links:

/product
/additive
/allergen
/category
/cgi/search.pl
/contributor
/country
/label
/manufacturing-place
/origin
/packaging
/state
/store

teolemon commented 2 years ago

We run Google AdGrants campaigns (free Adwords for NGOs), and the console says deeplinking will improve conversion rates:

g123k commented 2 years ago

I would like this PR to be merged first: https://github.com/flutter/packages/pull/2416 It will ease the migration.

Can we wait for it? Or should I start with a fork containing it?

monsieurtanuki commented 2 years ago

Before any code is started, I would like to understand the goal, the strategy, and the impact of waiting/not waiting for https://github.com/flutter/packages/pull/2416 to be merged. I'm old-fashioned, sorry about that ;)

As far as I understand, the goal is to allow deep-linking, and the most relevant way to do that is "named routes" or "router 2.0". Btw are both solution equivalent? Is that just the same concept with a different name?

Does that mean that

all current Smoothie pages are supposed to be accessed via a named route, e.g. /history? Of course there are parameters too I guess, like /product/1234
we have to connect "manually" Smoothie named routes and the website URLs via some regexp?
same implementation on Android and iOS?
is there a way we can code that gradually (in successive PRs), like convert to named routes first, then to router 2.0 (if needed/relevant), then deep linking on Android, then deep-linking on iOS?

g123k commented 2 years ago

So let me explain: to achieve the goal of having deep-links, the best solution is to migrate to Navigator 2.0. As we all know, Navigator 2.0 requires hundreds of lines of code and the use of external libraries is encouraged by Google itself. The most popular solution is from an ex-Google employee: go_router.

I had started a few weeks ago an implementation (never pushed, my fault), but I was soon stuck with codes like this:

var res = Navigator.of(context).push()
// Do something with res

Basically go_router doesn't allow receiving a result from a closing route. The only is by tricking and making the code totally awful and unreadable. And the PR I mentioned finally fixes that issue.

monsieurtanuki commented 2 years ago

@g123k Thank you for your answer!

That's why I wanted to discuss the matter before any code writing: it looks like what causes problems is:

either the choice of Navigator 2.0
or the fact that, if we choose Navigator 2.0, we cannot pop/return a value (before the merge you indicated anyway)

My comments (a bit late in the evening to my standards) are:

perhaps there is one less problematic solution than Navigator 2.0 (I'm just speculating)
or perhaps we should rethink our current use of pop in order to return a value - either by not returning values and just notifying listeners, or by using a slightly different solution for specific pages that really need to return a value (that could be converted to dialogs?), a bit like what happened today with @simonbengtsson's WillPopScope where 2 specific solutions were implemented for the problematic 2 specific cases.

The first step would be to list the pop that return a value, and see what we could do with them. Assuming that they can be converted to dialogs, we won't have to wait for the flutter merge. And we can start migrating to Navigator 2.0, then (or simultaneously) deep-linking.

How does that sound?

monsieurtanuki commented 2 years ago

Working on step one: transforming all await Navigator.push<MyObject?>( calls into await Navigator.push<void>( when the pop/return value is not used.

Current status:

% grep -r "await Navigator.push" *
cards/data_cards/image_upload_card.dart:          final bool? refreshed = await Navigator.push<bool>(
cards/product_cards/smooth_product_card_not_found.dart:                    final String? result = await Navigator.push<String>(
cards/product_cards/product_title_card.dart:                await Navigator.push<Product?>(
cards/product_cards/smooth_product_card_found.dart:            await Navigator.push<Widget>(
pages/scan/scan_header.dart:                      await Navigator.push<Widget>(
pages/scan/scan_product_card.dart:    await Navigator.push<Widget>(
pages/product/edit_product_page.dart:                      await Navigator.push<Product?>(
pages/product/edit_product_page.dart:                      final bool? refreshed = await Navigator.push<bool>(
pages/product/edit_product_page.dart:                      await Navigator.push<Product?>(
pages/product/edit_product_page.dart:                      await Navigator.push<Product?>(
pages/product/edit_product_page.dart:                      await Navigator.push<Product?>(
pages/product/edit_product_page.dart:        await Navigator.push<Product>(
pages/product/edit_product_page.dart:        await Navigator.push<Product>(
pages/product/product_image_gallery_view.dart:        final File? photoUploaded = await Navigator.push<File?>(
pages/product/add_ingredients_button.dart:          await Navigator.push<bool>(
pages/product/add_nutrition_button.dart:          await Navigator.push<Product>(
pages/product/common/product_list_page.dart:                    await Navigator.push<Widget>(
pages/product/summary_card.dart:              await Navigator.push<Product?>(
pages/product/summary_card.dart:                await Navigator.push<Widget>(
pages/product/new_product_page.dart:              await Navigator.push<void>(
pages/product/add_category_button.dart:          await Navigator.push<Product>(
pages/product/add_new_product_page.dart:          final File? finalPhoto = await Navigator.push<File?>(
pages/product/add_new_product_page.dart:          final Product? result = await Navigator.push<Product?>(
pages/product/add_new_product_page.dart:          final Product? result = await Navigator.push<Product?>(
pages/all_user_product_list_page.dart:                    await Navigator.push<void>(
pages/question_page.dart:                            await Navigator.push<Widget>(
pages/user_management/login_page.dart:                            final bool? registered = await Navigator.push<bool>(

M123-dev commented 2 years ago

Just FYK the mentioned PR is closed (not merged)

monsieurtanuki commented 2 years ago

We should not merge this until we figure out this how to support imperative API moving forward https://github.com/flutter/flutter/issues/99112. Closing for now

Thank you @M123-dev! Looks like we need to clean our code first and figure out how to distinguish "edit dialogs with returned values" from "real pages". First step is reviewing #2799 ;)

monsieurtanuki commented 2 years ago

Now I need your opinion guys.

We still have 8 cases where "pages" are pushed and a returned value is expected:

cards/data_cards/image_upload_card.dart:          final bool? refreshed = await Navigator.push<bool>(
cards/product_cards/smooth_product_card_not_found.dart:                    final String? result = await Navigator.push<String>(
pages/product/edit_product_page.dart:                      final bool? refreshed = await Navigator.push<bool>(
pages/product/product_image_gallery_view.dart:        final File? photoUploaded = await Navigator.push<File?>(
pages/product/add_new_product_page.dart:          final File? finalPhoto = await Navigator.push<File?>(
pages/product/add_new_product_page.dart:          final Product? result = await Navigator.push<Product?>(
pages/product/add_new_product_page.dart:          final Product? result = await Navigator.push<Product?>(
pages/user_management/login_page.dart:                            final bool? registered = await Navigator.push<bool>(

We need to code those 8 cases differently - as they are now we won't be able to switch to named routes (where no value is returned). And if you have a look at those 8 cases, it's about a place where the user inputs data (e.g. editing a product or logging in). Which has nothing to do with deep-linking - unless we want to be able to connect directly to the "edit product picture 15" page, but from what I saw in the Android URL list this is not the case.

Therefore I suggest to switch those 8 cases from a page to a dialog or - perhaps better - to a bottom sheet.

`MaterialPageRoute` / `fullscreenDialog: true` (current mode)	`showModalBottomSheet` / `isScrollControlled: true` (suggested mode)

The UI difference is minimal, and it takes 1 minute to switch from one mode to the other.

Are you guys OK with this idea of converting pages that return values, to bottom sheets? After that we would be able to migrate to named routes, or Navigator 2.0.

M123-dev commented 2 years ago

AFAIK named routes and Router 2.0 implementations like go_router are completely different and for deeplinking we likely need router 2.0 so we can skip named routes. I just checked and it looks like we can (with what seems like replacing routes) indeed somewhat pop a value

https://stackoverflow.com/questions/71359432/flutter-go-router-how-to-return-result-to-previous-page

https://gorouter.dev/user-input

monsieurtanuki commented 2 years ago

@M123-dev If you ask me, from what I read I prefer the dynamic named routes solution: it's easier to implement, and it allows deep-linking all the same. That said, I barged into this issue 4 days ago because I assumed that there was something I could be helpful with, considering that pages returning values were a problem (cf. @g123k and https://github.com/flutter/packages/pull/2416) If actually that's not a problem that's good news, I'm done here. Now you guys are all set for the implementation!

teolemon commented 2 years ago

https://android-developers.googleblog.com/2022/08/monitor-your-deep-links-in-one-place.html?m=1&s=09

teolemon commented 2 years ago

FYI: https://github.com/flutter/packages/pull/2416, they want to change stuff before they merge, so it's going to take a little more time

M123-dev commented 2 years ago

Heyy just one little detail I've seen in the Medium article for flutter 3.3 go_router now supports async push.

The latest version (4.3) enables apps to redirect using asynchronous code, and includes other breaking changes described in the migration guide.

Can't say if that allows to pop values but still interesting to know

teolemon commented 2 years ago

"It reduces the amount of code needed", dixit Edouard

teolemon commented 1 year ago

@monsieurtanuki does the move to Flutter 3.7 change things on this one ?

monsieurtanuki commented 1 year ago

@teolemon I think we were alright even before, as since months ago we no longer expect pop'ed values from our navigator calls. Except for minor cases that are now handled as "full screen dialog"s rather than as pages.

g123k commented 1 year ago

If that's ok for you, it will take back this issue and implement it. I will start with the products.

monsieurtanuki commented 1 year ago

@g123k That's perfect: I started 30 minutes ago with only preliminary works, and I reassign this issue to you.

For what it's worth:

% grep -r "builder: (BuildContext context) =>" *
cards/data_cards/image_upload_card.dart:          builder: (BuildContext context) => ProductImageGalleryView(
cards/product_cards/smooth_product_card_not_found.dart:                    builder: (BuildContext context) =>
cards/product_cards/smooth_product_card_found.dart:                  builder: (BuildContext context) => ProductPage(product),
knowledge_panel/knowledge_panels/knowledge_panel_card.dart:              builder: (BuildContext context) => SmoothBrightnessOverride(
pages/preferences/user_preferences_connect.dart:                  builder: (BuildContext context) => ScaffoldMessenger(
pages/preferences/user_preferences_connect.dart:                      builder: (BuildContext context) => Scaffold(
pages/preferences/user_preferences_dev_mode.dart:              builder: (BuildContext context) => SmoothAlertDialog(
pages/preferences/user_preferences_dev_mode.dart:                builder: (BuildContext context) => const OfflineDataPage(),
pages/preferences/user_preferences_dev_mode.dart:              builder: (BuildContext context) => const OfflineTaskPage(),
pages/preferences/user_preferences_dev_mode.dart:              builder: (BuildContext context) =>
pages/preferences/user_preferences_food.dart:      builder: (BuildContext context) => SmoothAlertDialog(
pages/preferences/user_preferences_account.dart:          builder: (BuildContext context) => const LoginPage(),
pages/preferences/user_preferences_account.dart:              builder: (BuildContext context) => const LoginPage(),
pages/preferences/user_preferences_account.dart:                builder: (BuildContext context) => AccountDeletionWebview(),
pages/preferences/user_preferences_account.dart:                  builder: (BuildContext context) => const LoginPage(),
pages/preferences/user_preferences_user_lists.dart:          builder: (BuildContext context) => const AllUserProductList(),
pages/preferences/abstract_user_preferences.dart:          builder: (BuildContext context) => UserPreferencesPage(
pages/scan/scan_header.dart:                          builder: (BuildContext context) =>
pages/scan/scan_product_card.dart:        builder: (BuildContext context) => ProductPage(product),
pages/scan/search_page.dart:        builder: (BuildContext context) => ProductPage(fetchedProduct.product!),
pages/product/nutrition_add_nutrient_button.dart:          builder: (BuildContext context) => StatefulBuilder(
pages/product/add_simple_input_button.dart:              builder: (BuildContext context) => SimpleInputPage(
pages/product/nutrition_page_loaded.dart:        builder: (BuildContext context) => NutritionPageLoaded(
pages/product/edit_product_page.dart:                      builder: (BuildContext context) =>
pages/product/edit_product_page.dart:                      builder: (BuildContext context) => EditOcrPage(
pages/product/edit_product_page.dart:                      builder: (BuildContext context) => EditNewPackagings(
pages/product/edit_product_page.dart:                      builder: (BuildContext context) => EditOcrPage(
pages/product/edit_product_page.dart:            builder: (BuildContext context) => SimpleInputPage(
pages/product/edit_product_page.dart:            builder: (BuildContext context) => SimpleInputPage.multiple(
pages/product/add_ocr_button.dart:              builder: (BuildContext context) => EditOcrPage(
pages/product/product_image_viewer.dart:        builder: (BuildContext context) => SmoothAlertDialog(
pages/product/product_image_viewer.dart:        builder: (BuildContext context) => UploadedImageGallery(
pages/product/product_image_viewer.dart:          builder: (BuildContext context) => CropPage(
pages/product/common/product_refresher.dart:      builder: (BuildContext context) => SmoothAlertDialog(
pages/product/common/product_refresher.dart:                builder: (BuildContext context) => const LoginPage(),
pages/product/common/product_dialog_helper.dart:                  builder: (BuildContext context) => AddNewProductPage(barcode),
pages/product/common/product_dialog_helper.dart:        builder: (BuildContext context) => SmoothAlertDialog(
pages/product/common/product_query_page.dart:                  builder: (BuildContext context) => PersonalizedRankingPage(
pages/product/common/product_query_page_helper.dart:        builder: (BuildContext context) => ProductQueryPage(
pages/product/summary_card.dart:                      builder: (BuildContext context) =>
pages/product/summary_card.dart:                        builder: (BuildContext context) =>
pages/product/summary_card.dart:        builder: (BuildContext context) => KnowledgePanelPage(
pages/product/new_product_page.dart:                    builder: (BuildContext context) =>
pages/product/new_product_page.dart:                  builder: (BuildContext context) =>
pages/product/add_new_product_page.dart:            builder: (BuildContext context) => AddBasicDetailsPage(
pages/image/uploaded_image_gallery.dart:                  builder: (BuildContext context) => CropPage(
pages/all_user_product_list_page.dart:                        builder: (BuildContext context) =>
pages/image_crop_page.dart:    builder: (BuildContext context) => StatefulBuilder(
pages/image_crop_page.dart:      builder: (BuildContext context) => CropPage(
pages/product_list_user_dialog_helper.dart:        builder: (BuildContext context) => _UserEmptyLists(daoProductList),
pages/product_list_user_dialog_helper.dart:      builder: (BuildContext context) => _UserLists(
pages/user_management/sign_up_page.dart:      builder: (BuildContext context) => SmoothAlertDialog(
pages/user_management/login_page.dart:                              builder: (BuildContext context) =>
pages/user_management/login_page.dart:                                builder: (BuildContext context) =>
pages/onboarding/onboarding_flow_navigator.dart:      builder: (BuildContext context) => getPageWidget(context, page),
pages/onboarding/onboarding_flow_navigator.dart:        builder: (BuildContext context) => SmoothScaffold(
smooth_category_picker_example.dart:                        builder: (BuildContext context) =>
widgets/tab_navigator.dart:          builder: (BuildContext context) => child,

% grep -r "builder: (final BuildContext context) =>" *
pages/offline_tasks_page.dart:                      builder: (final BuildContext context) =>
pages/preferences/user_preferences_dev_mode.dart:      builder: (final BuildContext context) => SmoothAlertDialog(
pages/product/portion_calculator.dart:      builder: (final BuildContext context) => SmoothAlertDialog(
pages/product_list_user_dialog_helper.dart:      builder: (final BuildContext context) => SmoothAlertDialog(
pages/product_list_user_dialog_helper.dart:      builder: (final BuildContext context) => SmoothAlertDialog(

M123-dev commented 1 year ago

What kind of solution do you plan to use @g123k a self build one or a package like go_router

g123k commented 1 year ago

What kind of solution do you plan to use @g123k a self build one or a package like go_router

My idea is to use go_router, as it's the most commonly used package.

M123-dev commented 1 year ago

Sounds good 👍🏼

g123k commented 1 year ago

The config file (assets links) is now deployed on the website https://github.com/openfoodfacts/openfoodfacts-server/pull/8306#event-8963728420

openfoodfacts / smooth-app