filelist only refreshed if directory changes by noveens · Pull Request #27282 · owncloud/core

noveens · 2017-03-01T12:00:31Z

Description

Fixes #15520
Fixes #26318

The filelist is refreshed if and only if the directory is changed and not otherwise.

Related Issue

#15520
#26318

Motivation and Context

How Has This Been Tested?

Tested at all directories.

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)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2017-03-01T12:00:34Z

@noveens, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @butonic and @Henni to be potential reviewers.

PVince81 · 2017-03-01T12:49:09Z

apps/files/js/filelist.js

 			if (!force && currentDir === targetDir) {
 				return;
 			}
+			if(this._currentDirectory && currentDir === targetDir) {


it seems this is what the previous if statement is doing.

Note that there is a force attribute to let callers force a refresh. With your approach this would break.

Did you have a look if it is possible to add this check much earlier, maybe inside app.js ?

@PVince81 ,
I had a look, it's not possible to do this at a previous state.

Have a look in FileList._onUrlChanged. Try adding the check there instead.

noveens · 2017-03-01T13:17:37Z

@PVince81 ,
updated changes, please review :-)

PVince81 · 2017-03-01T13:18:49Z

apps/files/js/filelist.js

 		_onUrlChanged: function(e) {
 			if (e && _.isString(e.dir)) {
+				var currentDir = this.getCurrentDirectory();
+				if(this._currentDirectory && currentDir === e.dir) {


Yes, looks better. Does it work ?

Also, why did you add this._currentDirectory in the check ? What's the purpose ?

Yes, this works now as well.

The this._currentDirectory check is there because we wouldn't want to skip file loading at the root directory ( '/' ).

if we are at root directory OR home page the initialised values of currentDir and e.dir turn out to be same, hence the file-loading doesn't complete at the root page.
To solve this, I introduced the check.

Ah, I see. That makes sense. Please add a comment in the code to clarify this.

So basically this._currentDirectory is null/empty when the file list is first initialized

Exactly.
Adding a comment.

PVince81 · 2017-03-01T13:38:38Z

Now curious if some unit test will fail.

Would be good to have a JS unit test for that case too.

noveens · 2017-03-01T13:40:00Z

@PVince81 ,
For which case are you talking about?

PVince81 · 2017-03-01T13:43:57Z

For which case are you talking about?

The case you just fixed here. Add the following tests for FileList:

if triggering the event for url changed with the same as the current path, the file list is not refreshed (changeDirectory is not called, you can use a stub to check that)
if triggering the event for url changed with the root path directly after FileList creation, the file list is refreshed (changeDirectory is called, that's the case you explained in the code comment)

I'll first let you look at the "fileListSpec.js" file to find existing tests you could based this on.
Let me know if you get lost there.

noveens · 2017-03-01T13:51:29Z

@PVince81 ,
On it.

PVince81 · 2017-03-01T16:25:23Z

apps/files/js/filelist.js

 				var currentDir = this.getCurrentDirectory();
 				// this._currentDirectory is NULL when fileList is first initialised
-				if(this._currentDirectory && currentDir === e.dir) {
+				if( (this._currentDirectory || this.$el.find('#dir').val()) && currentDir === e.dir) {


no, don't use the old crappy '#dir' element. This should be the same value like this.getCurrentDirectory() already.

This is neccesary, otherwise the first test fails
in the test, this._currentDirectory remains null for some reason.

Then we should rather fix the test instead. Maybe something is missing in the test initialization.

should I remove this then and look into why it's not initialised?

Yes please.

PVince81 · 2017-03-01T16:26:20Z

apps/files/tests/js/filelistSpec.js

+
+		beforeEach(function() {
+			if (fileList) {
+				fileList.destroy();


You don't need to destroy here, remove this. The afterEach in a higher level at the top of the file already takes care of this.

PVince81 · 2017-03-01T16:26:36Z

apps/files/tests/js/filelistSpec.js

+			if (fileList) {
+				fileList.destroy();
+			}
+			fileList = new OCA.Files.FileList($('#app-content-files'), {


also no need to create, the very first beforeEach does that already.

PVince81 · 2017-03-01T16:26:45Z

apps/files/tests/js/filelistSpec.js

+			fileListStub.restore();
+		})
+		it('File list must be refreshed', function() {
+			console.log(fileList.getCurrentDirectory());


remove this

oh, sorry :p

PVince81 · 2017-03-01T16:27:11Z

apps/files/tests/js/filelistSpec.js

+		it('File list must be refreshed', function() {
+			console.log(fileList.getCurrentDirectory());
+			$('#app-content-files').trigger(new $.Event('urlChanged', {dir: '/'}));
+			expect(fileListStub.notCalled).toEqual(false);


use fileListStub.calledOnce here, it's better

should that be called exactly once?

Yes. Enforcing exactly once is also a good practice.

Just in case future bugs would make it be called multiple times...

PVince81 · 2017-03-01T16:28:53Z

apps/files/tests/js/filelistSpec.js

 			testMountType(123, 'external-root', 'external', 'external');
 		});
 	});
+	describe('file list should not refresh if url does not change', function() {


You only need one describe block to group your two tests.

describe('file list url refresh tests', function() { beforeEach(...); afterEach(...); it('does not reload the file list if path in url does not change', ...); it('refreshes the file list after list creation', ...); });

noveens · 2017-03-01T19:05:11Z

@PVince81 ,
As per your suggestion of removing the test for homepage refresh,
It turned out that if I remove this.$el.find('#dir').val() , the test for not refreshing when directory hasn't changed seems to fail and not the other way round.
Maybe you can look into this further?

Thanks.

noveens · 2017-03-01T19:06:04Z

@butonic ,
can you please review?

PVince81 · 2017-03-02T08:19:07Z

@noveens I looked into it and it seems that it's more complicated than I thought. We can't easily get rid of '#dir' at this point. So I'm willing to accept the PR as it is, with the '#dir' check inside the event handler, for now.

If you also agree, feel free to merge this directly as the tests pass 👍

PVince81 · 2017-03-02T08:27:02Z

apps/files/tests/js/filelistSpec.js

+		});
+		afterEach(function() {
+			fileListStub.restore();
+		})


one more thing: missing semicolon here.

If you run jshint/jslint on JS files it will tell you that. Maybe your IDE has an option for this.

noveens · 2017-03-02T08:43:39Z

@PVince81 ,
Many thanks for the review :-)
I updated the changes, it would be great if you could merge this instead of me this time :-)

Thanks.

lock · 2019-08-03T20:02:08Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

filelist only refreshed if directory changes

70fd099

PVince81 reviewed Mar 1, 2017

View reviewed changes

check introduced at another method

8042534

PVince81 reviewed Mar 1, 2017

View reviewed changes

noveens added 2 commits March 1, 2017 19:10

comment added to explain one check

780c35c

comment added to explain one check

f7ef0c9

PVince81 added the 2 - Developing label Mar 1, 2017

PVince81 added this to the 10.0 milestone Mar 1, 2017

unit tests added

f636291

PVince81 reviewed Mar 1, 2017

View reviewed changes

small fixes in unit tests

d7ccd94

PVince81 reviewed Mar 2, 2017

View reviewed changes

missing semicolon added

064d4fa

PVince81 added 4 - To release and removed 2 - Developing labels Mar 2, 2017

PVince81 merged commit e06b804 into master Mar 2, 2017

PVince81 deleted the 15520 branch March 2, 2017 11:01

PVince81 mentioned this pull request Mar 2, 2017

Closing photo preview reloads file list #26318

Closed

oparoz mentioned this pull request Mar 2, 2017

File list sometimes refreshes after closing picture or video viewer nextcloud/server#659

Closed

MorrisJobke mentioned this pull request Mar 3, 2017

filelist only refreshed if directory changes nextcloud/server#3686

Merged

noveens mentioned this pull request Mar 5, 2017

after closing the image, the images list will stay in the same position. owncloud/gallery#716

Closed

8 tasks

PVince81 mentioned this pull request Apr 20, 2017

File lists do not properly reload when switching #27697

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019

Conversation

noveens commented Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

mention-bot commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noveens commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noveens Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noveens Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Mar 1, 2017

Uh oh!

noveens commented Mar 1, 2017

Uh oh!

PVince81 commented Mar 1, 2017

Uh oh!

noveens commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noveens commented Mar 1, 2017

Uh oh!

noveens commented Mar 1, 2017

Uh oh!

PVince81 commented Mar 2, 2017

Uh oh!

noveens commented Mar 1, 2017 •

edited

Loading

noveens Mar 1, 2017 •

edited

Loading

noveens Mar 1, 2017 •

edited

Loading