openfoodfacts / smooth-app

🤳🥫 The new Open Food Facts mobile application for Android and iOS, crafted with Flutter and Dart
https://world.openfoodfacts.org/open-food-facts-mobile-app?utm_source=off&utf_medium=web&utm_campaign=github-repo
Apache License 2.0
853 stars 282 forks source link

[Discussion] App modularization #2591

Closed g123k closed 1 year ago

g123k commented 2 years ago

Hi everyone,

We now have a few pending issues regarding:

Dart (and Flutter) doesn't allow us to have dynamic dependencies, like Gradle would on Android. The only solution is to have an architecture that looks like this:

Arch

To achieve this kind of architecture, Flutter forces us to specify the name of the package for each asset we want. That's why, we have to change the current "app" to be the common module. This will force us to provide a package for all assets we want to access. (That's what I've done in #2320, but not only)

Here is the plan:

  1. Dismiss the current PR #2320
  2. Create a PR that only adds package everywhere in the common module + create an app to test the architecture
  3. Split the barcode scanner in modules: MLKit vs another "non Google" solution
  4. Create two variants of the app

If you have any suggestion, feel free to ask!

M123-dev commented 2 years ago

I don't know of a better way to do this if we don't have official flutter support.

I mean we could ask if there is a maybe undocumented way or if it's on the todo list but if not I'm fine with doing it this way.

monsieurtanuki commented 2 years ago

Hi @g123k!

If I understand well:

  1. we need to build different apps for Android, because our current version uses Google packages that we cannot/we don't want to put in FDroid or Amazon stores
  2. it's not possible to have a unique smart pubspec.yaml that - according to env variables for instance - would build either the google or the amazon version

I'm a bit puzzled because somehow we manage to override the app version from the pubspec.yaml (0.0.0+600) at build time, so perhaps we could manage to go a step further and override dependencies too. But perhaps not, perhaps we can change only few things, among which app name and version.

So let's assume points 1 and 2 above are correct.

For the moment smooth_app is our main package. And if I understand well you want to rename it as common (or just consider as the "common" package, that's not clear).

What if we said that

I don't know if that would work. My point here is to have a minimal impact on the existing code. @g123k I know you're at ease with 99-file PRs like #2320, I'm not a big fan of that (I prefer 99 Luftballons, but that's another story).

So basically, @g123k could you test if we could have

g123k commented 2 years ago

So you're right. Let's we rename the current smooth_app to common (but we can keep the same name). Since it's not the root package anymore, we HAVE TO link all assets manually by specifying the package argument (mainly what's done in #2306

For the tests, no need actually, because I'm 100% sure (I've done it in previous projects). We will just have to change the entrypoint from smooth_app/main.dart to smooth_app_android|amazon/main.dart

monsieurtanuki commented 2 years ago

Hi @g123k!

There are some things I would like to clarify before you start.

Let's rename the current smooth_app to common (but we can keep the same name)

=> Please do keep the same name. That was the only important point of my previous comment. We need it to keep the code history (at least I do).

Since it's not the root package anymore, we HAVE TO link all assets manually by specifying the package argument (mainly what's done in https://github.com/openfoodfacts/smooth-app/pull/2306

Very surprising indeed. That would entail that if I use a pub.dev package that uses its own assets, I would have to rename those package asset files? Obviously some misunderstanding: we need to clarify this. Especially when https://github.com/openfoodfacts/smooth-app/pull/2306 has nothing to do with it.

In short, as a test, you would just have to

The next step would be 1) to create an abstract class in smooth_app, something like that:

abstract class AbstractScanner {
  void helloWorld();
}

2) to extend that class in smooth_fdroid, something like that:

class FdroidScanner extends AbstractScanner {
  @override
  void helloWorld() => print('hi this is Fdroid here');
}

3) to pass an instance of FdroidScanner as a parameter to smooth_app/main.dart

4) to start the FDroid app and find hi this is Fdroid here in the logs.

g123k commented 2 years ago

If you download a dependency from pub.dev and this dependency have some assets, you have to specify the package entry. Otherwise Flutter won't know which file to take.

and that's supposed to work, with 0 change on smooth_app, right?

No, because of the package thing

monsieurtanuki commented 2 years ago

@g123k I have to test that. Sounds very strange. I'll keep you posted by tomorrow.

g123k commented 2 years ago

If you want to better understand why, just decompile an Android APK and you will thus understand that package question

monsieurtanuki commented 2 years ago

@g123k We're probably not talking about the same thing.

If I use a package that I find in pub.dev, and if this package contains assets, I do not have to change that package assets, do I? Like for https://github.com/flutter/packages/tree/main/third_party/packages/cupertino_icons/assets - I don't think you ever changed the path to an icon: Capture d’écran 2022-07-18 à 19 22 54

Probably the source of our misunderstanding is what we want to do in the old package and the new packages. The way I see things, the old stays as is with its assets. And the new packages just call the old package with an additional parameter (like a class that implements the scan), but that's all they do. In particular, they don't play with assets, they're calling a package that happens to have its own assets.

Still in my tests.

g123k commented 2 years ago

Let me rephrase everything. Let's say we have:

When in your base app, you say Image.asset(path), it automatically implied package = base_app => Current behavior, everything works well (but the package could be provided - actually done by Flutter itself)

Now, Cupertino_icons have its own font asset, but let's say it's an image. If I say Image.asset(path) => 404, because the asset is in a subfolder called cupertino_icons and NOT in the base app.

So if I sum up, in the code (wherever you are), if you want to access an asset that is defined outside the main app, you have to specify the package.

That's what we have to do for smoothie, since the base app will now become a "dependency".

monsieurtanuki commented 2 years ago

if you want to access an asset that is defined outside the main app

I never will. I will just call a black box - a.k.a. dependency. A black box that uses its own assets. The main app is never going to access directly the dependency assets.

What you imply is that a dependency does not have the same access to its own assets, depending on whether the dependency is a dependency to another package or is independent. That's possible, but that would be very surprising to me. I'll continue my tests tomorrow.

g123k commented 2 years ago

No, what I just say, is:

From the Flutter docs:

To fetch an asset from a package, the package argument must be provided. For instance, suppose the structure above is inside a package called my_icons. Then to fetch the image, use:

AssetImage('icons/heart.png', package: 'my_icons')

monsieurtanuki commented 2 years ago

@g123k My tests today:

  1. in a first approach it looks like you may be right regarding assets. Very surprising - not the fact that you are right, this I can imagine, but that a black box like a dependency needs to be (slightly) rewritten. But if at the end this is how it works, we can manage that in a stupid PR that just adds the path to all asset calls.
  2. what is much more problematic is the way flutter manages the translations (it's notoriously an area where flutter sucks). It looks like as the translations are computed from the current package folder where you flutter run - e.g. smooth_fdroid, all the translations need to be at the level of that folder, as there's only one generated app_localizations.dart file. That would mean that we would need to copy the app_xx.arb files for each fdroid/amazon/google project. Which definitely sucks, much more than point 1

@g123k What are your thoughts about localization?

teolemon commented 2 years ago

Regarding localization, can we keep everything in a single file ? The 160*4 files for server and the old Android app are a nightmare.

monsieurtanuki commented 2 years ago

Regarding localization, can we keep everything in a single file ?

You mean, a unique file for all the flutter apps, or a unique file for all languages?

monsieurtanuki commented 2 years ago

@g123k Regarding assets (again), I've just a look at pub.dev packages that contain assets (not that easy to find). I found this guy: https://pub.dev/packages/intl_phone_field that contains flags. And this is how they access the "local" assets:

Image.asset(
  'assets/flags/${_filteredCountries[index].code.toLowerCase()}.png',
  package: 'intl_phone_field',
  width: 32,
)

Same with https://pub.dev/packages/country_pickers and https://pub.dev/packages/country_list_pick.

OK, you win :)

We'll have to check if all our asset cases (json, svg, png, ogg, pem, txt, ttf) can be dealt with with that additional package parameter.

But as I said, the localizations part looks more problematic.

M123-dev commented 2 years ago

I think the best solution would be to just avoid the problem as we are now in a stage where there are not many people with the same problems.

Preferably by somehow running the generation step before flutter run does it's job like its possible in JS with scripts in the counterpart to darts pubspec.yaml

The closest thing I could find is this: https://flutterawesome.com/define-and-use-scripts-from-your-pubspec-yaml-file/amp/

For this to work we would still need a command which generates the translations files without running flutter run looked for it multiple times already but haven't found one.


Edit: I just checked again and it's flutter gen-l10n maybe I should get more in the habit of reading the docs instead of Stack Overflow

M123-dev commented 2 years ago

Also for other assets there seem to be workarounds https://stackoverflow.com/questions/54740732/how-to-add-assets-in-flutter-package-plugin-development

  1. Moving the ./assets into ./lib/assets

or

  1. AssetImage('assets/abc.xyz', package: 'my_developed_package');
monsieurtanuki commented 2 years ago

@teolemon @g123k @M123-dev You guys realize that ALL that heavy non-sense could be prevented if we were just able to comment and uncomment the pubspec.yaml and a single dart file, right? Either manually or automatically:

  #  smooth_mlkit: ### to be commented/uncommented
  #    path: ../../../smooth_mlkit ### to be commented/uncommented
  smooth_zxing: ### to be commented/uncommented
    path: ../../../smooth_zxing ### to be commented/uncommented

In both smooth_mlkit and smooth_zxing, we create one class called ScanInterface (that possibly implements a common abstract class)

And in smooth_app's main.dart, we just call once ScanInterface:

// import 'package:smooth_mlkit/smooth_mlkit.dart'; ### to be commented/uncommented
import 'package:smooth_zxing/smooth_zxing.dart'; ### to be commented/uncommented

final ScanInterface scanInterface = ScanInterface();

Et voilà.

M123-dev commented 2 years ago

That should be doable without that much work automatically

g123k commented 1 year ago

I think we can close this issue as it's now implemented