isDir always false, so code has no effect

[brownrb]

in FSBrowser.ino

Line 195 sets isDir false
bool isDir = false;

then a test in line 197 tests the state of isDir which will always be false
output += (isDir) ? "dir" : "file"

Nowhere is the value of isDir changed, so this code has an error.

[Jeroen88]

Maybe bool isDir = dir.isDirectory() will work

[francescolavra]

Indeed the line:

bool isDir = false;

should be changed to:

bool isDir = entry.isDirectory();

(The OP is talking about the ESP8266WebServer example sketch at https://github.com/esp8266/Arduino/blob/master/libraries/ESP8266WebServer/examples/FSBrowser/FSBrowser.ino)

[brownrb]

This fails for ESP8266 because entry.isDirectory() is invalid for ESP8266

[francescolavra]

entry.isDirectory() returns always false if you use SPIFFS (because of a limitation in SPIFFS), but should work if you use LittleFS.

[earlephilhower]

@brownrb would you like to do a PR with this small change, since you found it?

[brownrb]

What is a PR

[d-a-v]

Pull Request on github
Merge Request elsewhere
It's a patch proposal

[earlephilhower]

@brownrb , could you generate a pull request with the fix? You've found the bug and solution, and it would be a good way to get your name in the commit logs for posterity. :)

[brownrb]

What is a pull request? On Monday, February 10, 2020, 06:19:56 AM GMT+13, Earle F. Philhower, III <[email protected]> wrote: @brownrb , could you generate a pull request with the fix? You've found the bug and solution, and it would be a good way to get your name in the commit logs for posterity. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[earlephilhower]

A quick test with LittleFS and directories shows the fix to be more complicated than it looks.

Replacing with isDir = entry.isDirectory() does result in the specific results working (i.e. the JSON this pseudi-cgi generates will contain 'dir's), but the actual web page JavaScript (minified so it's not human readable) doesn't know what to do with an entry of type 'dir' so simply drops directories from the list.

So, the fix needs to also incorporate a new .js file that can handle subdirs. It's a can of worms. :(

[vdeconinck]

If nobody is working on this, I can try to fix it because this FSBrowser is a little gem I include in all my projects and I'm also facing issues migrating to LittleFS due to it.
So unless there's already effort going on this topic, I can try to unminify the code and take a look at it (don't hold your breath though)
@me-no-dev , seems you were the original author of this sample. Would you by chance still have the original JS source ?

[earlephilhower]

@vdeconinck that would be great!

I'm curious, though, if it is worth the effort. We might want to drop this and just have the SDFSBrowser instead. I think I've hacked it personally with just a few changes to run on LittleFS (since SD and LittleFS and SPIFFS now have the exact same API, it's almost just a matter of changing the startup code to call LittleFS.begin())...

[vdeconinck]

@earlephilhower Ahem I can't seem to find SDFSBrowser anywhere (searched my Arduino install folder, searched Github, even asked Google to no avail). Care to point me to that file ?
In the meantime, I checked me-no-dev's alternate version included in the ESPAsyncWebServer project which is a bit more recent, but it is even more obfuscated as the edit.htm.gz file is encoded as an array in the C source, https://github.com/me-no-dev/ESPAsyncWebServer/blob/master/src/SPIFFSEditor.cpp - Yay ;-)

[earlephilhower]

Sorry, had the wrong name. It's this: https://github.com/esp8266/Arduino/tree/master/libraries/ESP8266WebServer/examples/SDWebServer

If you change the SD.begin() with LittleFS.begin() it almost works (I think I had 1 more change somewhere), and has subdir support.

[vdeconinck]

Thanks. I played a bit with the SDWebServer example I had completely overlooked till now, and modifications to make it work with LittleFS are minor indeed, so I would advise the OP to switch to that version:
@brownrb , starting from SDWebServer, replace all occurrences of "SD." by "LittleFS.", remove the pin parameter to begin(), add a "r" parameter to single-parameter versions of open(), and replace FILE_WRITE by "w", and you should be good)
@earlephilhower Do you think it would be welcome to try and make that code a "FS-agnostic" file manager, with a #define logic to select the filesystem type ? Probably the naming of the example should be changed too... Just tell me.

[earlephilhower]

I think that would be a good PR, @vdeconinck . In that PR, you can delete the FSBrowser example since it's being superseded.

[vdeconinck]

OK. I'll try to wrap up something clean and tested on all 3 filesystems.
I will also switch from the old "SD" to the new "SDFS" as recommended in the documentation.

[vdeconinck]

I'm making good progress. Two questions though:

• Although the javascript includes the ace text editor, I can't actually save the modifications if I edit a file in the browser. I don't think it is linked to the FS changes, and I don't think it ever worked, but was it meant to ?
• And a stupid one : what should be the name of that "universal" filesystem browser ?
  I'm thinking of either recycling "FSBrowser" or going for WebFileManager (because "browser" can be confused with "web browser" while "File Manager" I think is more descriptive).
  But I'm also open to any other name you might think of. Opinions ?

[devyte]

can't actually save

I can confirm that it does save when used with spiffs. I use it regularly to develop my app's web interface.

[vdeconinck]

OK, thanks. I must have broken something. I'll check the difference with the original code.

[vdeconinck]

Re: editor: I admit I never realized it was an actual editor. I always thought it was a viewer :-). CTRL-S did work and still works ;-).
For the rest, I can't stop improving things ;-). Current work is on my repo, but not ready for prime time yet.
In particular, I have an issue with the notion of "filesystem root": On LittleFS/SDFS, openDir("") and openDir("/") return the same set of files, but it's not the case on SPIFFS, where files such as "hello.txt" are not displayed in the current FSBrowser because they don't start with "/".
I'm nearly done supporting filenames with no leading slash on SPIFFS, but I now realize that people could end up uploading for example "index.htm", not realizing that it won't be served because it's not "/index.htm" (just got bitten by it myself, doh).
What do you think ?
Should I keep on ignoring files with no leading slash, or support them and cause confusion ?

[devyte]

About improving things: please keep in mind that one use case for the esp8266 is in isolation, i. e. not connected to the internet. That means no access to 3rd party js libs.

[vdeconinck]

@devbyte Err, that code already relies on ace.js being downloaded from a CDN on the internet...
But I'm not adding any dependency, just plain JS.

[devyte]

That's exactly what I was aiming for, thanks!
One thing you could do is to list the current dependencies in a README (ace is a direct one, but there are other indirect ones), so that a user who wants to run in isolation could get them manually. Or maybe some tool for that already exists?
About SPIFFS, please keep in mind that the underlying lib maintenance has been stagnated for a while now, so we're deprecating it in favor of LittleFS. That means: I suggest keeping status quo, and not investing significant effort into developing features related to SPIFFS.

[vdeconinck]

Thanks for not holding your breath ;-) Hope you'll like this unified version "on steroids".

[earlephilhower]

Closing as fixed thanks to #7182