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

[Meta] Migrate to Single-Activity Compose Navigation architecture #1172

Open
ArnyminerZ opened this issue Dec 13, 2024 · 5 comments
Open

[Meta] Migrate to Single-Activity Compose Navigation architecture #1172

ArnyminerZ opened this issue Dec 13, 2024 · 5 comments
Labels
refactoring Internal improvement of existing functions

Comments

@ArnyminerZ
Copy link
Member

ArnyminerZ commented Dec 13, 2024

Right now we are using the multi-activity architecture that was the standard for a long time, but it looks like everything is moving to a single-activity with navigation (at least it's how it's explained in the docs).

In the migration guide, the first thing they recommend is to "Move screen-specific UI logic out of activities", which we have already done pretty much everywhere, so this is completed. The next step would be of course, start getting rid of the activities in order to migrate into a single-activity.

I don't think this process should be done in a single movement, but maybe from less to more complex. First move activities that don't have any intent-filters, and are directly related to each other. For example, AboutActivity can be merged with AccountsActivity, and the same goes for IntroActivity.

PermissionsActivity is for example a complex one, because it's related to multiple other activities, so I'd wait to migrate it.

I'd split it into the following steps.

The idea is obviously at the end of the process, AccountsActivity stops existing and we have a proper MainActivity, that holds the main navgraph, and redirects accordingly to the request made.


First Phase (no cross-dependencies)

I'll put first the activity that will hold the other ones, and below the ones that would be merged into it.

  • AccountsActivity
    • IntroActivity
    • AboutActivity
  • AppSettingsActivity
    • TasksActivity
  • AccountActivity
    • CollectionActivity
    • CreateAddressBookActivity
    • CreateCalendarActivity
  • WebdavMountsActivity
    • AddWebdavMountActivity

Second Phase (AppSettingsActivity)

This activity is defined as parent for DebugInfoActivity, PermissionsActivity and TasksActivity.

TasksActivity

Should already be merged into the Activity by this point, so no worries here.

DebugInfoActivity

It has IntentBuilder defined inside, so this would have to be moved into AppSettingsActivity. All the extras can be moved into the companion of AppSettingsActivity, and I'd probably create a new action named something like at.bitfire.davdroid.ui.debug_info, so that if AppSettingsActivity is launched with this action, it will be automatically redirected to the destination of the "old" DebugInfoActivity passing all the extras.

Then, all the functions in DebugInfoActivity would be moved into a viewmodel (shareZipFile, shareFile, viewFile).

All usages of DebugInfoActivity and DebugInfoActivity.IntentBuilder would of course be moved to AppSettingsActivity.

PermissionsActivity

It is used in multiple places, but doesn't have any extras, so we would only have to add an action, for example at.bitfire.davdroid.ui.permissions, which if given to AppSettingsActivity sets the navgraph location to the contents of PermissionsScreen.

@ArnyminerZ ArnyminerZ added the enhancement New feature or request label Dec 13, 2024
@rfc2822
Copy link
Member

rfc2822 commented Dec 16, 2024

I don't think this process should be done in a single movement, but maybe from less to more complex. First move activities that don't have any intent-filters, and are directly related to each other. For example, AboutActivity can be merged with AccountsActivity, and the same goes for IntroActivity.

Good plan, but I think it shouldn't be named AccountsActivity then. At the end of every phase, every activity, screen etc. should have a correct name. For instance, if we can't continue right after the first phase and then come back in a few months, there will be a a confusing naming scheme of the activity in the meanwhile which makes it hard to fix bugs etc.

So I'm for a new MainActivity and then migrate the activities one-by-one.

We must however pay extra attention to activities/intents which can be launched from outside. For instance the login Intent or the Account intent. And of course all the screens that take arguments from a PendingIntent / notification.

@rfc2822
Copy link
Member

rfc2822 commented Jan 1, 2025

We could try starting this mega-issue 😅

I suggest to

  • start with a new issue + PR that converts one activity (AccountsActivity? or something more easy, perhaps without arguments) to MainActivity, and as soon as this PR is merged,
  • remove the Activities one by one (1 PR per activity/screen).

Then we can also work on multiple activities in parallel. What do you think @ArnyminerZ @sunkup?

@ArnyminerZ Would you like to start with the MainActivity PR?

@rfc2822 rfc2822 added refactoring Internal improvement of existing functions and removed enhancement New feature or request labels Jan 1, 2025
@rfc2822 rfc2822 changed the title [Draft] Migrate to Single-Activity Compose Navigation architecture [Meta] Migrate to Single-Activity Compose Navigation architecture Jan 1, 2025
@sunkup
Copy link
Member

sunkup commented Jan 2, 2025

I suggest to

  • start with a new issue + PR that converts one activity (AccountsActivity? or something more easy, perhaps without arguments) to MainActivity, and as soon as this PR is merged,
  • remove the Activities one by one (1 PR per activity/screen).

Then we can also work on multiple activities in parallel. What do you think @ArnyminerZ @sunkup?

Sounds good to me.

We must however pay extra attention to activities/intents which can be launched from outside. For instance the login Intent or the Account intent. And of course all the screens that take arguments from a PendingIntent / notification.

I take it that apps which start certain activities (Etar, JtxBoard, Tasks.org) might have to make changes to how they send users to certain parts of the DAVx5 app. Or would the reference, default to the new MainActivity? Otherwise we could notify the app developers about the breaking changes?

@rfc2822
Copy link
Member

rfc2822 commented Jan 2, 2025

I take it that apps which start certain activities (Etar, JtxBoard, Tasks.org) might have to make changes to how they send users to certain parts of the DAVx5 app. Or would the reference, default to the new MainActivity? Otherwise we could notify the app developers about the breaking changes?

Yes, I'd prefer to

  • provide a legacy wrapper that allows the old entry points / activites to be called and redirect to the new MainActivity somehow,
  • change the documentation to the new way and notify related apps about it
  • remove the legacy wrapper at some time in the future when everyone has switched.

@sunkup
Copy link
Member

sunkup commented Jan 3, 2025

List of known references, to check they don't break:

  • Nextcloud app ("sync contacts/calendars with DAVx5")
  • jtxBoard (has refernences in this SyncUtil file)
    • AccountsActivity: Drawermenu > Collections > 3-dotmenu > Add remote collection (DAVx5)
    • AccountActivity: Drawermenu > Synchronization > Go to collections
    • ...
  • tasks.org
    • ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Internal improvement of existing functions
Projects
None yet
Development

No branches or pull requests

3 participants