Page size calculation based on real page height

[nazar-pc]

This is fix for #10060
Instead of hard coding page size as 20 items, we check real page height, and divide by 50 (height of one row).
This will allow to load fewer items on small screens and enough items on large screens (4k, portrait orientation, etc.).
Also checking page height on every load to respond on browser window resizing,

[ghost]

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[ghost]

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[nazar-pc]

I'm agree this to be public domain)

[karlitschek]

@PVince81

[PVince81]

@owncloud-bot ok to test

[ghost]

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng/346/:bomb: Test FAILed. 💣

[PVince81]

Please use this.$el instead of the absolute selector (I believe it should be the same element)

[nazar-pc]

Fixed by using this.$el.parent() to get access to proper element.

[PVince81]

Thanks a lot for this nice fix! 😄

I suspect this will break the unit tests which are expecting a page size of 20.
You'll need to use sinon.stub() to stub the pageSize function in the unit tests before the construction here:
https://github.com/owncloud/core/blob/master/apps/files/tests/js/filelistSpec.js#L124

Like this:
pageSizeStub = sinon.stub(OCA.File.FileList.prototype, 'pageSize').returns(20);

Then in afterEach():
pageSizeStub.restore();.

Let me know if you need further help.
To run the JS unit tests: http://doc.owncloud.org/server/7.0/developer_manual/core/unit-testing.html#javascript-unit-testing-for-core

[ghost]

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[PVince81]

I wonder whether this.$scrollContainer would be better.
This is because sometimes the container isn't the same, for example on the "public link" page.

Have you tried the public link page with your fix, for a folder with many files ? (share with link)

(I haven't tested your PR yet)

[nazar-pc]

@PVince81, all tests locally finished without errors, can you check once again?

[PVince81]

See #11590

[nazar-pc]

@PVince81, I think you meant this.$container by this.$scrollContainer.
Applied that, public link page works fine now, tests are OK.

[PVince81]

Great, the code looks good. Thanks.
I'll test this soon.

This will need another reviewer @owncloud/designers @MorrisJobke

[PVince81]

@MorrisJobke just heard you had a portrait mode screen, ideal to test this 😉

[MorrisJobke]

just heard you had a portrait mode screen, ideal to test this

;)

[MorrisJobke]

Works 👍 (But I don't even had problems on master) But this approach is much more generic.

[nazar-pc]

I have 2160px height in landscape mode, and 3840px in portrait (4k monitor), so, probably, you didn't reach limits.

[oparoz]

@nazar-pc, thanks for fixing this! Just tested this all-in-one and it worked for me with browser windows in portrait mode.
https://github.com/nazar-pc/core/compare/patch-1.patch

[MorrisJobke]

I have 1536 px in portrait mode. And I cherry-picked the commits to stable7 (as I can reproduce the issue there) and it fixes the issue -> thanks for the fix

[PVince81]

Did you guys all test both the main file list and public sharing page ?

The code looks good 👍

[MorrisJobke]

@PVince81 I just checked the main file list.

[PVince81]

Just tried it out in normal mode (not portrait) and it still works correctly.

Running ./autotest-js.sh doesn't seem to work for me so they might need fixing.

[nazar-pc]

Fixed, tests doesn't fail anymore.

[scrutinizer-notifier]

The inspection completed: No new issues

[PVince81]

Excellent. Let's wait for Jenkins to give green light then this can be merged.

[MorrisJobke]

@owncloud-bot retest this please

[ghost]

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/352/

Build result: FAILURE

GitHub pull request #11524 of commit 8198e70 automatically merged.Building remotely on vm-slave-02 (SLAVE) in workspace /var/jenkins/workspace/pull-request-analyser-ng-simple > git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Fetching upstream changes from https://github.com/owncloud/core.git > git --version # timeout=10 > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/11524/merge^{commit} # timeout=10Checking out Revision ce79b57a2aac7c8dce6c8cde695aa79f33e6acb0 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f ce79b57a2aac7c8dce6c8cde695aa79f33e6acb0 > git rev-list c37ebe42efe2574fa2b829aff57528d9e5412a8a # timeout=10 > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » vm-slave-02pull-request-analyser-ng-simple » vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 2 second
💣 Test FAILed. 💣

[PVince81]

@owncloud-bot retest this please

[PVince81]

Jenkins still clumsy...

[ghost]

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/355/

Build result: FAILURE

GitHub pull request #11524 of commit 8198e70 automatically merged.Building remotely on vm-slave-02 (SLAVE) in workspace /var/jenkins/workspace/pull-request-analyser-ng-simple@3 > git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Fetching upstream changes from https://github.com/owncloud/core.git > git --version # timeout=10 > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/11524/merge^{commit} # timeout=10Checking out Revision ce79b57a2aac7c8dce6c8cde695aa79f33e6acb0 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f ce79b57a2aac7c8dce6c8cde695aa79f33e6acb0 > git rev-list c37ebe42efe2574fa2b829aff57528d9e5412a8a # timeout=10 > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » vm-slave-02pull-request-analyser-ng-simple » vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 2 second
💣 Test FAILed. 💣

[PVince81]

Forget Jenkins... I'll run the tests locally.

[PVince81]

JS tests run locally (this is a JS-only PR) -> merging

[PVince81]

@karlitschek backport ?

[nazar-pc]

I realized that 7.0.3 for whatever reason doesn't contain this fix, what happened? I still have 20 items in list.

[nazar-pc]

And 7.0.2 too

[PVince81]

Seems we missed backporting this bugfix @karlitschek @craigpg

[karlitschek]

Please backport. after 7.0.4 Thanks

[PVince81]

CC @jnfrmarks

[oparoz]

@nazar-pc I did add a patch to the OC 7.x forums as soon as I saw it wasn't included
http://forum.owncloud.org/viewtopic.php?f=29&t=24872

[nazar-pc]

Just made cherry-pick of this pull request merge into stable7: #12624

[MorrisJobke]

@nazar-pc Thanks. But we need to wait until the 7.0.4 freeze is over and 7.0.4 is released.

[nazar-pc]

So, it may get into 7.0.5?

[LukasReschke]

Correct