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

[full-ci] Improve performance when loading folders and share indicators #8349

Merged
merged 16 commits into from
Feb 7, 2023

Conversation

JammingBen
Copy link
Collaborator

@JammingBen JammingBen commented Feb 1, 2023

Description

  • When adding/removing shares, we update the sharesTree manually instead of re-loading it completely
  • When loading share indicators, we rely on the shareType property on the resource (for direct shares) and on the parent folders (for indirect shares).
  • The sharesTree is now only loaded when accessing the sidebar (before, it was loaded as soon as you navigated into a folder, as well as on resource creation).

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

To-Dos

  • Fix depth: 0
  • Fix getParentFolders() usage in FileDetails
  • Remove !!this.currentFolder?.shareTypes?.length check in CreateAndUpload.vue
  • Only fetch shareType and path when listing all parent folders
  • Move re-usable parts into composable
  • Properly load parent folders (where?)
  • Testing

@JammingBen JammingBen self-assigned this Feb 1, 2023
@update-docs
Copy link

update-docs bot commented Feb 1, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@ownclouders
Copy link
Contributor

ownclouders commented Feb 1, 2023

Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/32508/31/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingInternalGroupsSharingIndicator-shareWithGroups_feature-L65.png

webUISharingInternalGroupsSharingIndicator-shareWithGroups_feature-L65.png

webUISharingInternalGroupsSharingIndicator-shareWithGroups_feature-L86.png

webUISharingInternalGroupsSharingIndicator-shareWithGroups_feature-L86.png

webUISharingInternalGroupsToRootSharingIndicator-shareWithGroups_feature-L58.png

webUISharingInternalGroupsToRootSharingIndicator-shareWithGroups_feature-L58.png

webUISharingInternalGroupsToRootSharingIndicator-shareWithGroups_feature-L74.png

webUISharingInternalGroupsToRootSharingIndicator-shareWithGroups_feature-L74.png

webUISharingInternalUsersSharingIndicator-shareWithUsers_feature-L104.png

webUISharingInternalUsersSharingIndicator-shareWithUsers_feature-L104.png

webUISharingInternalUsersSharingIndicator-shareWithUsers_feature-L127.png

webUISharingInternalUsersSharingIndicator-shareWithUsers_feature-L127.png

webUISharingInternalUsersToRootSharingIndicator-shareWithUsers_feature-L52.png

webUISharingInternalUsersToRootSharingIndicator-shareWithUsers_feature-L52.png

webUISharingInternalUsersToRootSharingIndicator-shareWithUsers_feature-L69.png

webUISharingInternalUsersToRootSharingIndicator-shareWithUsers_feature-L69.png

@JammingBen JammingBen force-pushed the refactor-shares-indicator-loading branch 3 times, most recently from 32b7583 to e4f428f Compare February 2, 2023 16:56
@JammingBen JammingBen changed the title [full-ci] Refactor share indicator loading without re-loading the shares tree [full-ci] Refactor share indicator loading without using the shares tree Feb 2, 2023
@JammingBen JammingBen force-pushed the refactor-shares-indicator-loading branch from e4f428f to 45a1b29 Compare February 2, 2023 16:58
@JammingBen
Copy link
Collaborator Author

JammingBen commented Feb 3, 2023

Status: The indicator loading when adding or removing shares still has some fllaws. We also need the parent folders here, so that needs to be implemented properly first. Where do we want to load the parent folders? How do we persist them?

Edit: See #8383

@JammingBen JammingBen force-pushed the refactor-shares-indicator-loading branch from 45a1b29 to 882fb2c Compare February 7, 2023 09:20
@JammingBen JammingBen force-pushed the refactor-shares-indicator-loading branch from 882fb2c to 6954df2 Compare February 7, 2023 09:23
@JammingBen JammingBen changed the title [full-ci] Refactor share indicator loading without using the shares tree [full-ci] Refactor shares & indicator loading to improve performance Feb 7, 2023
@JammingBen JammingBen changed the title [full-ci] Refactor shares & indicator loading to improve performance [full-ci] Improve performance when loading folders and share indicators Feb 7, 2023
@JammingBen JammingBen marked this pull request as ready for review February 7, 2023 12:25
@@ -0,0 +1,6 @@
Enhancement: Improve performance when loading folders and share indicators
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@@ -254,8 +255,11 @@ export default defineComponent({
}

watch(
selectedFiles,
() => [...unref(selectedFiles), props.open],
(newResource, oldResource) => {
Copy link
Member

Choose a reason for hiding this comment

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

The callback args newResource and oldResource are not a list of resources anymore, because you added an additional attribute in the watcher. They are now arrays of resources and an additional boolean. That breaks the check in line 275 on !!oldResource.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, thx! I removed it completely, as we don't need it anymore. There is still room for some improvements when navigating with an opened sidebar, but it's already 100 times better than before.

@kulmann
Copy link
Member

kulmann commented Feb 7, 2023

cc @pascalwengerter @diocas @elizavetaRa this PR removes the ugly loading of shares via ocs in the file list (for the share indicators) and switches to super slim propfinds on parent folders. I think this should be pretty noticeable in cernbox. @JammingBen also tried to make the ancestor propfinds non-blocking (and re-rendering the rows that have a share indicator), but that was not very promising even with Vue3. If you want to try that for cernbox I'd love to hear about your experience.

@sonarcloud
Copy link

sonarcloud bot commented Feb 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

52.5% 52.5% Coverage
2.1% 2.1% Duplication

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

❤️

@JammingBen JammingBen merged commit ddd2371 into master Feb 7, 2023
@delete-merged-branch delete-merged-branch bot deleted the refactor-shares-indicator-loading branch February 7, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants