Ok @lognaturel @seadowg it's time to decide how to handle Mapbox 10. As we talked recently options are:

• sharing our tokens so that everyone can build the app including CircleCi
• storing Mapbox 10 in our local repository
• having a separate flavor

I was more in favor of one of two first options because it would be easier. Then if it doesn't work for us or there is any problem with licenses we can add a separate flavor.
Please think about it one more time and we can discuss it.

@grzesiek2010 is the compilation error on CI expected? I'm guessing it's because there isn't a Mapbox API key configured to grab the SDK.

sharing our tokens so that everyone can build the app including CircleCi
storing Mapbox 10 in our local repository

I think these feel roughly equivalent: both would work but might result in Mapbox suspending our account/revoking keys, and we'd have to resort to doing something else eventually. An alternative to this that still keeps the build simple would be to require CI (which could be leaked easily unless we stop running CI for PRs) and developer to all have their own Mapbox account/API keys and create their own local secrets.properties.

having a separate flavor

I still think something along these lines is my preferred option. We can add a product flavour for when Mapbox is included and when it isn't and have the former include the real implementation of MapboxMapFragment (and other Mapbox classes) and have the latter include a stub one.

As I've thought about it more however, I think there might be a "simpler" way to do achieve the same goals: move Mapbox code to a module (we want to do this anyway for all the different MapFragment implementations) and only include it when we have an access token for the SDK. As an example, in our dependencies for collect_app we'd have something like :

if (mapboxAccessToken != null) {
    implementation project(':mapbox')
}

This would allow us to have a module that would only compile when mapboxAccessToken is present. In other words, it's OK if it doesn't compile because of a missing dependency. Then when we're creating the Fragment in collect_app we can just load the class dynamically rather than statically:

try {
    return this.getClassLoader().loadClass("org.odk.collect.mapbox.MapboxMapFragment").newInstance();
} catch (ClassNotFoundException e) {
     return null;
} catch (IllegalAccessException e) {
     return null;
} catch (InstantiationException e) {
     return null;
}

If we do go down this road (which I'm advocating for), we probably want to look at merging #5139 before this as those changes will be needed to allow MapboxMapFragment to be pulled out to a separate module.

is the compilation error on CI expected? I'm guessing it's because there isn't a Mapbox API key configured to grab the SDK.

exactly there are no MAPBOX_DOWNLOADS_TOKEN so the app can't be built

Are you able to roll back to Kotlin conversions here? It makes it hard to understand the actual diff. For instance, Git is unable to track the changes you had to make to MapboxInitializationFragment d858661.

I'm not sorry. Generally I always implement changes and once everything is done I add conversion in a separate commit but not this time sorry my mistake. To help you a bit with that class what I changed was:

• I've removed onStart/onResume/onPause/onStop/onSaveInstanceState/onLowMemory/onDestroyView methods because now it's handled automatically https://docs.mapbox.com/android/maps/guides/migrate-to-v10/#simplified-lifecycle-management
• I've updated loading styles https://docs.mapbox.com/android/maps/guides/migrate-to-v10/#map-styles

and that's it.

Just a note about something to keep in mind: Chromebooks/Chrome OS devices are pretty likely to have x86. That's a real world example of x86 devices that we didn't really consider the first time we removed Mapbox's x86 libs (and therefore x86 support). I imagine we can get away with not supporting Mapbox on Chromebooks, but we should make sure the release build can start OK on them.

@seadowg
I moved mapbox code to its own module.
I wanted to try load classes dynamically like you said above: #5141 (comment)

The problem is that we use that new module in more places than only to create MapboxMapFragment like in your example.
I'm going to list those places so that it's clear:

• creating MapboxMapFragment in https://github.com/getodk/collect/pull/5141/files#diff-f424798f5410bd1996038d93031423c03974cb72ff81112f1a58be8a03c228f0R21 we should be ale to handle this in the way you presented above
• creating MapboxMapConfigurator in https://github.com/getodk/collect/pull/5141/files#diff-3a00df4aebd5600fae250c3807e97417704f10649504cc5f3cad20652bfc2a3fR61 the same as above
• using MapboxMapConfigurator.DEFAULT_MAP_STYLE in Defaults I can replace it with a raw string mapbox://styles/mapbox/streets-v11 so that mapbox module is not used there
• MapBoxInitializationFragment in main_menu this would need to load that fragment dynamically so that if the mapbox module is not included there are no errors. BTW I need to double check if we still need this trick.
• places where we use classes from the new module to setup dagger:
  https://github.com/getodk/collect/pull/5141/files#diff-19e8ed4f040ab190989099eb37cd00c559b474b8e802b01260614bf9b628773bR12 https://github.com/getodk/collect/pull/5141/files#diff-a2f0be8cb9ce6618f33c478f6ae0c69fc5e6eefa5807833ffc3f1f324369c5b3R281 this might be a problem

what do you think?

creating MapboxMapConfigurator in https://github.com/getodk/collect/pull/5141/files#diff-3a00df4aebd5600fae250c3807e97417704f10649504cc5f3cad20652bfc2a3fR61 the same as above

Yeah, I think we can just dynamically add the Mapbox source option if the configurator is present.

using MapboxMapConfigurator.DEFAULT_MAP_STYLE in Defaults I can replace it with a raw string mapbox://styles/mapbox/streets-v11 so that mapbox module is not used there

👍

MapBoxInitializationFragment in main_menu this would need to load that fragment dynamically so that if the mapbox module is not included there are no errors. BTW I need to double check if we still need this trick.

Yes that sounds right. You'll have to add the Fragment with a transaction rather than loading it in XML using name.

places where we use classes from the new module to setup dagger:
https://github.com/getodk/collect/pull/5141/files#diff-19e8ed4f040ab190989099eb37cd00c559b474b8e802b01260614bf9b628773bR12 https://github.com/getodk/collect/pull/5141/files#diff-a2f0be8cb9ce6618f33c478f6ae0c69fc5e6eefa5807833ffc3f1f324369c5b3R281 this might be a problem

Ooooft yeah that's a problem. I think what we'll have to do there is give the dependency module a constructor with args, and then grab the constructor dynamically. So the module would look like this:

@Module
class MapboxDependencyModule(
    private val settingsProvider: SettingsProvider,
    private val networkStateProvider: NetworkStateProvider,
    private val referenceLayerRepository: ReferenceLayerRepository
) {
    @Provides
    fun providesSettingsProvider(): SettingsProvider {
        return settingsProvider
    }

    @Provides
    fun providesNetworkStateProvider(): NetworkStateProvider {
        return networkStateProvider
    }

    @Provides
    fun providesReferenceLayerRepository(): ReferenceLayerRepository {
        return referenceLayerRepository
    }
}

I'd been thinking that giving these modules constructors would be nice anyway - it prevents us from having to create Collect... subclasses, so it's just less code overall. You can construct that without a static reference like this in Java:

this.getClassLoader().loadClass("org.odk.collect.mapbox.MapboxDependencyModule").getConstructors()[0].newInstance(
    settingsProvider,
    networkStateProvider,
    referenceLayerRepository
);

Given we're using reflection to load and construct classes in multiple places, it might be a good idea to have a helper object that hides all this work. I could imagine a MapboxClassInstanceCreator that has methods like createDependencyModule and createMapboxInitializationFragment.

I've fixed all steps mentioned above apart from DI. As you know i will be off next two weeks so if this blocks you somehow feel free to take over.

What's left:

• fixing DI
• removing x86 libs
• making sure that everything works together

It feels like there's only a couple of minor problems left (assuming everything other than the last two issues posted by @srujner are fixed). I'm thinking we can go ahead and merge this to unblock other PRs and create issues for the remaining problems. Sound good @grzesiek2010? If so, @srujner could you create issues for your last couple of comments?

I'm not sure I would rather give the QA team some time till Monday at least if there is nothing big then we can file separate issues for everything.

Maybe y’all can check in when @seadowg starts his Monday? QA might have made good progress by then and it’d be great to unblock other map-related things in flight.

@grzesiek2010 just dismissed reviews so that I remember to have another look at changes. Just request a review when you're done.

Tested with Success!
Verified on Androids: 5.1, 10, 12

Verified cases:

• Issue #3883 is no longer reproducing;
• Five bugs will be created as separate issues, all minor things;
• Regression checks for GeoPoint, GeoTrace, GeoShape widgets;
• Regression checks on external geo widgets;
• Regression checks on Forms: geojson, gps_accuracy_threshold, map internal-select, placement_map, TestingGeoPointWidgetWithValue, OSM_buildings;
• Select Map forms;
• Offline Layers;
• Placement by tapping, manual mode and automatic mode
• Google Maps, Mapbox, OSM;
• Collecting Location in background;
• View Submissions on map;
• Sending Submission to Central;
• Switching between maps Google/OSM/Mapbox
• Map view with displayed points
• Screen rotation,
• Minimize app, using backward button,
• Light and Dark mode,

Tested with Success!
Verified on Androids: 8.1, 11

34089a4 yes it was an existing issue. the problem was that if we received current location before loading items on the map then other events like zooming to those points were ignored. However it wasn't visible on master at least on my device because somehow the process was a little bit faster (I think the new map fragment loads slower).

b2cea65 here again the code was broken but it worked well somehow and only now the bug is visible

So both issues existed but both of them were let's say hidden.

34089a4 yes it was an existing issue. the problem was that if we received current location before loading items on the map then other events like zooming to those points were ignored. However it wasn't visible on master at least on my device because somehow the process was a little bit faster (I think the new map fragment loads slower).

b2cea65 here again the code was broken but it worked well somehow and only now the bug is visible

So both issues existed but both of them were let's say hidden.

Thanks for the clarification. That makes sense. Can we pull those commits out for now (to a branch so that we don't lose the solutions you came up with), and then we can create issues or @getodk/testers can verify as part of a pass on #5164? That PR reworks SelectionMapFragment and adds a lot of tests to SelectionMapFragment so a rebase on top of these commits is going to be much harder than fixing the issues (and being able to write tests for them) there instead.

Can we pull those commits out for now

Those are pretty serious issues to me so I wouldn't release v2022.3 without it but if you want to just exclude it to avoid conflicts and then fix soon - I'm ok with it and I can do that.

@seadowg ok I reverted those two commits and I'm going to file separate issues for them.

Those are pretty serious issues to me so I wouldn't release v2022.3 without it but if you want to just exclude it to avoid conflicts and then fix soon - I'm ok with it and I can do that.

Thanks! Agreed, we need to fix before releasing.