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

Improve av. offline performance #4354

Merged
merged 9 commits into from
Apr 25, 2024
Merged

Conversation

Aitorbp
Copy link
Contributor

@Aitorbp Aitorbp commented Apr 4, 2024

Related Issues

App: #4197

  • Add changelog files for the fixed issues in folder changelog/unreleased. More info here
  • Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required)

QA

Checks:

Reports:

@Aitorbp Aitorbp self-assigned this Apr 4, 2024
@Aitorbp Aitorbp linked an issue Apr 4, 2024 that may be closed by this pull request
10 tasks
@Aitorbp Aitorbp force-pushed the feature/improve_av_offline_performance branch 2 times, most recently from ac5eee0 to 867bcde Compare April 4, 2024 09:11
@Aitorbp Aitorbp requested a review from JuancaG05 April 4, 2024 09:12
@Aitorbp Aitorbp force-pushed the feature/improve_av_offline_performance branch from 867bcde to 34621e4 Compare April 4, 2024 09:17
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes here to avoid code repetition @Aitorbp

@Aitorbp Aitorbp force-pushed the feature/improve_av_offline_performance branch 2 times, most recently from 507729a to 93b2c3d Compare April 8, 2024 06:53
@Aitorbp Aitorbp requested a review from JuancaG05 April 8, 2024 07:28
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now

@JuancaG05 JuancaG05 force-pushed the feature/improve_av_offline_performance branch from c05fdf5 to d86367b Compare April 9, 2024 06:32
@jesmrec
Copy link
Collaborator

jesmrec commented Apr 9, 2024

QA checks:

Feature:

  • Av offline file with no modification -> not synced
  • Av offline file with local modification -> synced
  • Av offline file with external modification -> synced
  • Av offline folder with no modification -> not synced
  • Av offline folder with local modification -> synced
    • add new file inside -> not synced till manual update (improve?) not regression
    • modify file inside
  • Av offline folder with external modification -> synced
    • add new file inside
    • remove existing file inside
    • modify existing file inside

Regressive sync use cases

  • When we try to choose the destination path where to make an auto-upload
  • When we copy a file, we choose the space, it makes a sync of the content.
  • When we do a move operation
  • When we open the application
  • When we click on set available offline on a folder/file in the operations menu
  • When we click on unset available offline on a folder/file in the operations menu -> NA
  • When switching accounts
  • When clicking on synchronize in the operations list -> improvement: show snackbar in av. offline
  • When entering a folder
  • When exiting the folder, by clicking on back
  • When cancelling a copy/move file operation
  • When triggering the AccountDiscoveryWorker worker
  • When the AvailableOfflinePeriodicWorker is activated, the files that are available offline are synchronised.
  • When we receive an external file and we choose the destination place.

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 9, 2024

(1) [FIXED]

Check the following steps out. We are losing the sync condition in the basic av. offline case.

  1. Create a folder and add some files inside (better images)
  2. Set the folder as av. offline (without browsing inside)
  3. Wait till everything download
  4. Unset the folder as available offline
  5. Remove locally the content of the folder
  6. Repeat step 2.
  7. After some seconds, remove device connection
  8. Browse into the folder and click on any file

Current: not posible to preview

Expected: file displayed. It is available offline, so the preview must be always available. When the folder was set as av. offline in step 2. all content inside had to be synced. It doesn't

Pixel 2 Android 11
d86367b9c

@Aitorbp
Copy link
Contributor Author

Aitorbp commented Apr 9, 2024

(1)

Check the following steps out. We are losing the sync condition in the basic av. offline case.

  1. Create a folder and add some files inside (better images)
  2. Set the folder as av. offline (without browsing inside)
  3. Wait till everything download
  4. Unset the folder as available offline
  5. Remove locally the content of the folder
  6. Repeat step 2.
  7. After some seconds, remove device connection
  8. Browse into the folder and click on any file

Current: not posible to preview

Expected: file displayed. It is available offline, so the preview must be always available. When the folder was set as av. offline in step 2. all content inside had to be synced. It doesn't

Pixel 2 Android 11 d86367b9c

With the change of refresh functionality, from now on only files that have been modified will be returned. This means that the local etag and the server etag are different. With this known, we are presented with the case discovered by @jesmrec.

This means if we delete the local copy and we want to make a set available offline, it will compare between two equal etags, as the file on the server has not changed and the local etag has remained as it was before deleting it.

So the condition has not detected that there was a change if we make the files available offline after deleting the content of the folder locally. This means that the download is not done even if it is available offline. If we take the connection offline and enter a file it will not be available.

So to solve this we must set the etag parameter to null if we want to delete files locally. This way, they will be downloaded again.

@Aitorbp Aitorbp force-pushed the feature/improve_av_offline_performance branch from 55082d0 to b1cad6f Compare April 9, 2024 16:18
@jesmrec
Copy link
Collaborator

jesmrec commented Apr 10, 2024

(2) [FIXED]

may be related with (1)

  1. Set a folder as av. offline
  2. Using other client, web f. ex., add more files to the folder
  3. In app, pull to refresh (not browsing inside)
  4. Remove device connection
  5. Try to open the file added in 2.

Current: file not opened

Expected: file should be opened. Pull to refresh action should trigger a sync in the av. offline folder because the etag changed after new item was added inside.

Pixel 2 Android 11
b1cad6fb

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 10, 2024

(3) [FIXED]

  1. Set a folder as av. offline
  2. Modify any file inside the folder using the open with option (i used a txt file with the quickedit free editor)
  3. Save modifications in external editor
  4. Go again to the app

Current: changes are not synced back to the server. Even pulling to refresh, changes not pushed

Expected: just entering the to the containing folder, changes are synced back to the server (checked current master)

Pixel 2 Android 11
d75ebeae0

@jesmrec jesmrec requested a review from JuancaG05 April 16, 2024 13:16
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 17, 2024

Go ahead with this! approved.

@Aitorbp Aitorbp force-pushed the feature/improve_av_offline_performance branch from b4377ce to 5f3759d Compare April 19, 2024 13:45
@Aitorbp Aitorbp force-pushed the feature/improve_av_offline_performance branch from 5f3759d to 1baca2c Compare April 25, 2024 11:07
@Aitorbp Aitorbp merged commit d8c04d7 into master Apr 25, 2024
5 checks passed
@Aitorbp Aitorbp deleted the feature/improve_av_offline_performance branch April 25, 2024 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve av. offline performance
3 participants