Skip to content

Accessing sharetab directly for a long list of files#35283

Merged
sharidas merged 1 commit into
masterfrom
access-directly-sharetab-issue
May 22, 2019
Merged

Accessing sharetab directly for a long list of files#35283
sharidas merged 1 commit into
masterfrom
access-directly-sharetab-issue

Conversation

@sharidas

Copy link
Copy Markdown
Contributor

Accessing sharetab directly for a long list of
files caused problems. The pages need to be fetched
appropriately to get the data. This changeset fixes
the problem.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

Lets say we have 30 files listed in the the UI. The maximum page size we have is 18. And hence when user tries to access the 19th file from the UI using http://localhost/testing3/index.php/apps/files/?dir=/test&&scrollto=file19.txt&details=shareTabView ( where file19.txt ) is the 19th file in the list, then the sharetab would be shown without any details. The reason for this is its the 19th element and hence the filelist doesn't have the tr added. So to solve this issue, the index of the file from the this.files is taken. And this would help to calculate the number of attempts required to fetch the page so that we get the data.

Related Issue

Motivation and Context

We need to fetch the pages to get the data populated, so that when they are accessed for the share tab field, the tab does not show empty.

How Has This Been Tested?

  • Create a folder test
  • Upload 150 files
  • Access 15th elements sharetab field
  • Access 110th file's share tab field
  • Access 150th files share tab field
  • Access 190th files share tab field
  • Access 210th files share tab field shows
    beyondarray

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@sharidas sharidas added this to the development milestone May 21, 2019
@sharidas sharidas self-assigned this May 21, 2019
@jvillafanez

Copy link
Copy Markdown
Member

Some questions:

  • Is there any element loaded before the scrollTo method is called? We don't really want to make unnecessary calls.
  • How does this affect to "normal" scrolling? You can have 500 files inside a folder, you enter the folder normally and start scrolling down to see the last element. I expect the minimum number of calls to the _nextPage() method needed to do so. I want to avoid having to call 3 times that method to get the 3rd page, and then 4 times to get the next one, and then 5 times to get the 5th page.

There are minor style things we can adjust in the code, but I'd prefer to check those couple of things first in case there are changes needed.

@sharidas

Copy link
Copy Markdown
Contributor Author
  • Is there any element loaded before the scrollTo method is called? We don't really want to make unnecessary calls.

The scrollTo method is called from setFiles method which calls _nextPage method once. Thats the one which populates the data where the call reaches updateDetailsView method. Inside updateDetailsView method the call this.findFileEl(fileName); would return nothing. And hence call to this.getModelForFile($tr); would return null making the model, in the code path null.

  • How does this affect to "normal" scrolling? You can have 500 files inside a folder, you enter the folder normally and start scrolling down to see the last element. I expect the minimum number of calls to the _nextPage() method needed to do so. I want to avoid having to call 3 times that method to get the 3rd page, and then 4 times to get the next one, and then 5 times to get the 5th page.

This does not affect normal scrolling. Because the code path is a little different, but eventually it calls nextPage method subsequntly while scrolling. So the containers scroll is binded to onScroll method ( in the same filelist.js file ). And this method calls nextPage method while user scrolls down. So lets say there are 100 files in a folder. The nextPage method from onScroll would be called 6 times.

Let me know if this addresses your query?

Comment thread apps/files/js/filelist.js Outdated
Comment thread apps/files/js/filelist.js Outdated
@sharidas sharidas force-pushed the access-directly-sharetab-issue branch from d76bd6a to eec6934 Compare May 22, 2019 11:08
@codecov

codecov Bot commented May 22, 2019

Copy link
Copy Markdown

Codecov Report

Merging #35283 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #35283      +/-   ##
============================================
- Coverage     65.54%   65.53%   -0.01%     
  Complexity    18647    18647              
============================================
  Files          1218     1218              
  Lines         70546    70546              
  Branches       1288     1288              
============================================
- Hits          46236    46229       -7     
- Misses        23933    23940       +7     
  Partials        377      377
Flag Coverage Δ Complexity Δ
#javascript 53.69% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.89% <ø> (-0.02%) 18647 <ø> (ø)
Impacted Files Coverage Δ Complexity Δ
lib/private/DB/Connection.php 65.44% <0%> (-0.74%) 49% <0%> (ø)
lib/private/Files/Storage/DAV.php 80.46% <0%> (-0.64%) 0% <0%> (ø)
lib/private/Files/View.php 84.3% <0%> (-0.31%) 398% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89b30c9...eec6934. Read the comment docs.

@codecov

codecov Bot commented May 22, 2019

Copy link
Copy Markdown

Codecov Report

Merging #35283 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #35283   +/-   ##
=========================================
  Coverage     65.54%   65.54%           
  Complexity    18647    18647           
=========================================
  Files          1218     1218           
  Lines         70546    70546           
  Branches       1288     1288           
=========================================
  Hits          46236    46236           
  Misses        23933    23933           
  Partials        377      377
Flag Coverage Δ Complexity Δ
#javascript 53.69% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.9% <ø> (ø) 18647 <ø> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64fb41f...d97ce08. Read the comment docs.

@sharidas sharidas force-pushed the access-directly-sharetab-issue branch from eec6934 to ec65d0b Compare May 22, 2019 13:32
Accessing sharetab directly for a long list of
files caused problems. The pages need to be fetched
appropriately to get the data. This changeset fixes
the problem.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@sharidas sharidas force-pushed the access-directly-sharetab-issue branch from ec65d0b to d97ce08 Compare May 22, 2019 13:54
@sharidas sharidas merged commit 6db604e into master May 22, 2019
@delete-merged-branch delete-merged-branch Bot deleted the access-directly-sharetab-issue branch May 22, 2019 16:06
phil-davis added a commit that referenced this pull request May 29, 2019
merge tests into Scenario Outline that were fixed by #35283
individual-it added a commit that referenced this pull request May 29, 2019
[stable10] merge tests into Scenario Outline that were fixed by #35283
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.

browsing directly to details does not work for files far down the list

2 participants