Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: 2.3.2
-
Fix Version/s: 2.3.3
-
Component/s: Filepicker
-
Labels:
-
Testing Instructions:
-
Affected Branches:MOODLE_23_STABLE
-
Fixed Branches:MOODLE_23_STABLE
-
Pull from Repository:
-
Pull Master Branch:
MDL-35664-master -
Pull Master Diff URL:
-
Rank:44401
Description
When viewing (not in tree view) a repository (Dropbox for example) and browsing into some subdirectories, clicking the refresh button will reload the root of the repository.
From what I have seen, the file picker is not storing the current path anywhere.
Also, when requesting the next page, the path sent back to repository_ajax is undefined.
And later on the following test will fail because the obj.path is the breadcrumb and scope.path is undefined:
// repository/filepicker.js:1036
if (scope.active_repo.hasmorepages && obj.list && obj.page &&
obj.repo_id == scope.active_repo.id &&
obj.page == scope.active_repo.page+1 && obj.path == scope.path) {
scope.parse_repository_options(obj, true);
scope.view_files(obj.list)
}
(I am not sure how this can be handled in the tree view.)
Issue Links
Activity
We had a long discussion of that with Fred. The problem seems to be much deeper, there are several points here:
1. Refresh button does not refresh current folder for repository with subfolders and jumps to the home folder
2. mode dynload=false is actually not used in core repositories so we did not so far hit all the problems with it. We need to create a dummy repository with subfolders and dynload=false, that has "files" with different extensions and check the following:
- Navigation and refresh button works correctly in all view modes
- When using with filter (i.e. only image files for tinymce) everything is filtered recursively - it seems that repository::prepare_listing() will break here
3. Pagination AND subfolders together won't really work in tree view but we can try it in icon/list views
2. Box.net uses dynload off, it might help testing.
3. The function requesting the next results should send the path we are currently on to avoid having to use $SESSION in the plugins.
I attach the repository_dummyrepo that has dynload=false and subfolders. Works terribly 
even worse. I tried to add
$list = array_filter($list, array($this, 'filter'));
at line 58 or repository_dummy and it seems that repository::filter does not work recursively too 
All right, this issue is much more complicated than it looks. The problem is that the non-dynamic loading (dynload=off) is missing functionalities.
The main problem being the breadcrumb, which while dynload on, is sent back after every directory listing, the dynload off does not. I thought I'd create a new parameter "breadcrumb" to send back as part of each level of the whole tree structure returned by the repository, but this is not really helping. The problem being that when we click on the breadcrumb, we will send a request to the repository and that's not what we want.
Leaving the breadcrumb problem on the side is a solution, but that's not fixing the problem. I personally think that the dynload off should be an option as some repositories have the ability to return the whole structure (or part of it) in one query, but without the user's ability to go back to (at least) the parent directory it is really user friendly (although that's the case in 2.2). I also think that a repository should be able to jump from one option to the other.
Another problem would be to consider that, when calling get_listing() with an empty $path, we are browsing the root of the repository. I think this would be a wrong assumption as the repository could handle an empty $path as some default $path. That's what I did in some repository I worked on, if the $path was empty, then the $path is considered as "somewhere/in/the/repository".
Those were my food for thoughts, I'll have another look at this issue again soon, but I think most of this issue would be an improvement instead of a bug and should maybe be addressed in another issue.
I personally think we should not support dynload=off mode at all. Just assume that it is always on and write about it in dev docs. It's too much work and not much use really.
At the same time repository may use caching to store last external request result.
Also when user clicks 'Refresh' button in filepicker the repository should receive some additional parameter that cache needs to be rebuilt
Pushing this for peer review again.
Fred, looks good to me
Thanks Marina. Pushing for integration.
Thanks Fred, a truly acceptable change. Has been integrated now
Test passed, all looking good. However, using box.net the directory path never changes and remains as root, and refreshing results in being taken back to the root folder. I was told by Fred that this is a separate issue.
Your issue has dug up some gold.
It works great i've been told.
Go forth, be brave, be bold.
yay! "All your thoughts are belong to everyone."
Thanks and ciao!
FYI this issue caused a regression in MDL-36696 with an additional comma causing IE to break (when in IE7 mode)
I am assigning this to me and Marina as peer reviewer as I am providing a patch.
I noticed that the file picker did not store the information about the path, and was living only with the information returned from the ajax requests. Therefore, I have added a variable req_path which returns the path that was requested by the user. I also updated the method that fetches the next results as the arguments passed were incorrect.