owncloud / android

:phone: The ownCloud Android App
GNU General Public License v2.0
3.76k stars 3.09k forks source link

[BUG] Request review in fresh install #4385

Closed Aitorbp closed 2 months ago

Aitorbp commented 2 months ago

Related Issues

App: https://github.com/owncloud/android/issues/4213


QA

Aitorbp commented 2 months ago

Below is the detail of the research that has been done to eliminate some unnecessary requests when installing the app from scratch:

Capabilities interactions when installing the application from scratch:

The capabilities are first called in the AuthenticationViewModel and then called again in the AccountDiscoveryWorker. This is redundant.

In the DrawerActivity the CapabilitiesViewModel is initialized. In the init of viewModel a refreshCapabilitiesFromNetwork() is done. Therefore, when the viewModel is initialized, a call will be made to the capabilities. Afterwards, in the FileDisplayActivity, the viewModel is initialized again, but koin detects that there is already an instance and does not execute the init again.

So first the capabilities are called once in the initialization of the viewmodel in the DrawerActivity and then it would be called again in the refreshCapabilitiesFromNetwork() call in the FileDisplayActivity. So the most correct thing would be to leave the init call to the capabilities and remove the one in the FileDisplayActivity, since it would be executed before the initialization of the DrawerActivity.

The result of the capabilities would look like this: When the application is started from scratch, the capabilities will be called twice: in the authenticatorViewModel and through the initialization of the CapabilitiesViewModel in the DrawerActivity. If we open the app again it will only be called from the DrawerActivity (this case is not tested because it doesn't give the App inspector time to see the requests that are sent). But searching the whole app there are only two observers to the capabilities: DrawerActivity and FileDisplayActivity.

Avatar interactions when installing the application from scratch:

The setupRootToolbar function contained in ToolbarActivity has a request to the loadAvatarForAccount(). This function is called from 3 places in the app:

In AccountManagerAdapter there is a call to the function loadAvatarForAccount() but it doesn't affect when you install the application from scratch. Not applicable.

The result of the avatar request will be as follows: One will be executed from FileDisplayActivity in the onCreated and one in the SyncProfileOperation in its syncUserProfile() function. We remove from the updateToolbar in the FileDisplayActivity that the avatar is called. Remember that this function is called 16 times in the class, but only in three of the calls it enters here when we install the app from scratch. In short, with these changes we reduce the avatar requests to two when the app is installed from scratch.

Sometimes when you run the application from scratch the request that is executed in SyncProfileOperation is made twice.... So we will have three calls from scratch.

App/list interactions when installing the application from scratch:

The app/list is always called in the refreshCapabilitiesAccount() when the filesAppProviders is not null. So, at the same with capabilities, it will call in the authenticatorViewModel and through the initialization of the CapabilitiesViewModel in the DrawerActivity. With all the above changes on an oc10 server, two calls to app/list are executed:

By removing the capabilities call in the AccountDiscoveryWorker the call to the app/list is no longer made at this point.

Interactions of files/admin when installing the application from scratch:

Runs in four places in the application when installing from scratch:

In short, the following image shows the requests that are made with the changes discussed above in an oc10 server:

oc10 requests from scratch The requests that are made with the changes discussed above in an ocis server:

ocis requests from scratch

jesmrec commented 2 months ago

Let's check the new status:

1. First launch

First launch oCIS

First launch oC10

the new status after fresh install is much better and approved on my side

2. Regressions

Before (4.2.1):

Opening uploads tab, a request to app/list and capabilities sent Opening shares tab, request to to shares Opening spaces tab, request to drives Opening drawer, no requests Opening settings, no requests Opening manage accounts, no requests

With such info, i will take the three relevant endpoints that were handled (capabilities, avatar and app/list) against relevant scenarios to send requests to them. This is the result:

Capabilities Avatar /app/list
Browsing No sent No sent No sent
Reopening Sent Sent only in oC10 Sent
Share Sent No sent Sent
Switch to uploads tab Sent No sent Sent
Switch from uploads tab Sent Sent only in oC10 Sent

Regards (no regressions):

In general, the status of this PR is stable. Just, the mentioned improvements are something we can live with but may deserve a quick look, in order to know if it is OK addressing to another issue or let them be... @Aitorbp

Aitorbp commented 2 months ago

Let's check the new status:

1. First launch

First launch oCIS

  • capabilities: 2 calls
  • avatar: no calls (does this endpoint exist in oCIS?)
  • app/list: 2 calls
  • PROPFIND to root: 1 call (recursive to discover)

First launch oC10

  • capabilities: 2 calls
  • avatar: 2 calls
  • app/list: 2 calls
  • PROPFIND to root: 1 call (recursive to discover)

the new status after fresh install is much better and approved on my side

2. Regressions

Before (4.2.1):

Opening uploads tab, a request to app/list and capabilities sent Opening shares tab, request to to shares Opening spaces tab, request to drives Opening drawer, no requests Opening settings, no requests Opening manage accounts, no requests

With such info, i will take the three relevant endpoints that were handled (capabilities, avatar and app/list) against relevant scenarios to send requests to them. This is the result:

Capabilities Avatar /app/list Browsing No sent No sent No sent Reopening Sent Sent only in oC10 Sent Share Sent No sent Sent Switch to uploads tab Sent No sent Sent Switch from uploads tab Sent Sent only in oC10 Sent Regards (no regressions):

  • When opening the Share option over any item, a call to app/list is triggered. I guess it could be something related with capabilities, please confirm. Sharing and app/list have nothing to do, so, it could be avoided.
  • When switching from any tab to Uploads , a call to capabilities and app/list is triggered. Same as before.
  • When switching from uploads tab to any other, a call to avatar is triggered. I guess it is because loading FileListActivity, right?

In general, the status of this PR is stable. Just, the mentioned improvements are something we can live with but may deserve a quick look, in order to know if it is OK addressing to another issue or let them be... @Aitorbp

In relation to what you say @jesmrec: The app/list is always called in the refreshCapabilitiesAccount() when the filesAppProviders is not null. Therefore, these requests that you indicate are being made. You should see if it is necessary to make that call even if filesAppProviders is different from null. I would leave it for another issue.

Finally, in reference to the avatar, when we are on the uploads screen, two calls are made to the avatar:

jesmrec commented 2 months ago

Thanks for the input. Behaviour was improved in comparison with the initial status and that was the target of the issue.

It can be merged into master