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

Dev Branch - fm.js, readIcons function requires DIRECTORYINDEX on Webserver #47

Closed
gmkll opened this issue Sep 14, 2016 · 7 comments
Closed
Assignees
Milestone

Comments

@gmkll
Copy link

gmkll commented Sep 14, 2016

In function readIcons in fileManager.js in branch dev/v1.0.6 the ajax requires directory read mode on the server side (DocumentIndex with Apache Webserver) as I understand it - which normally could not be expected and was not used/required in in v1.0.4 (BTW config.icons.path + '/' has by default a double slash at the end).

I would suggest a) remove it (i.e. just expect it) b) make it optional c) if the request fails with 404 then set a reasonable default.

I understand the intended flexibility, but why not use some already configured file extension list, e.g.
security.uploadRestrictions as "expected with fileType + '.png'
and assign it immediately in fm.js using e.g. the map function like
fileIcons = security.uploadRestrictions.map(function(x) { x+ ".png" }).

@psolom
Copy link
Owner

psolom commented Sep 14, 2016

Hm, I see.
Actually it was handled before with the option "c" that you have suggested. Take a look the "createImageUrl" function in this commit. As you can see there was a check "file_exists(fileTypeIcon)" and it worked good, but it produced 404 errors to browser console in case there was no icon found for file type. Not a good approach I believe. That why I have decided to read the whole list of icons from the filesystem, but you are right, it's not reliable way. There is option "d" - we can list all file types which have icons in config file, but of course it would be better to get the icons list dynamically somehow.

@gmkll
Copy link
Author

gmkll commented Sep 14, 2016

Hi @servocoder, yes, the .png files in images/fileicons could be set as default. Only if no match in fileIcons (and no match e.g. in "notFoundFIs", see below) an ajax request (with timeout) would be triggered and the result set into fileIcons if found. Otherwise (404/ajaxError) the fileType is marked as not found in an array e.g. named "notFoundFIs". Then this test would applied before triggering. This is kind of incremental dynamic and may be I think the best we can get from the client side.

@psolom
Copy link
Owner

psolom commented Sep 14, 2016

I don't think we need any ajax requests if we are going to list all possible filetype icons (see images/fileicons) in the config file. If there is no match in the list then default icon is applied. Of course we should keep the list up to date, not so flexible though. Perhaps I misunderstood your idea. What do we need ajax requests for, in the case I have described?

@gmkll
Copy link
Author

gmkll commented Sep 16, 2016

your are right, keep it simple, I agree!

@psolom psolom self-assigned this Sep 19, 2016
@psolom psolom added this to the 1.1.0 milestone Sep 19, 2016
@psolom
Copy link
Owner

psolom commented Oct 18, 2016

Hi @gmkll !
The issue is supposed to be fixed at the "dev" branch. I have moved all grid view icons paths to css, so there is no any conjuction with js side now. I would appreciate if you check it out and leave a feedback.

@gmkll
Copy link
Author

gmkll commented Oct 19, 2016

I skimmed the current dev branch (new version, see below), but detected (or was stopped by) some changes after merge pull #49. I think there are changes on the backend side also (I not inspected all PHP changes). E.g. the (new) resourceobject is expected to have an"attributes" JSON object (was Properties, as still refered to in the API), an id (was Path?), which I never realized before, ....

I´m comparing, what was version 1.0.4 with current version 1.0.6, which is also mentioned in filemanager.config.default.json, but I think this has to be updated to at least version 1.0.7 now. To handle API (breaking!) changes I would suggest to add this versioning to the API as well, which should be consistent with (or mapped to) the version in the JSON file. Otherwise it would be difficult to understand, what´s new/changed.

I stopped at this point..
Sorry, if I missed something?

@psolom
Copy link
Owner

psolom commented Oct 19, 2016

Oh, you use Java conneсtor, how could I forget.
It will require some changes in terms of API responses. You could find the discussion about API standardization at #37. The good news is that responses for all requests now looks similar, so that it's much clear at the moment. Of course the new API should be well described, so I will do that and come back when it's ready. Thank you for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants