Added local directory listing feature. #32167
Conversation
|
It looks like the test is failing: |
|
Yeah, I've realised a few problems with the unit test (and you've just spotted a new one):
I've added tiny non-empty files to the example sub-directories within Not sure yet how to get the correct path to appear in the expected HTML for the unit test, but I'll come up with something. |
|
I think the new unit tests should now pass successfully, but please do not approve/merge this yet because there is still a technical problem which I need advice in order to fix. Changes:The directory listing now groups sub-directories and files separately, and lists each in alphabetical order (well, whatever order Problems:I've realised that the directory listing renders fine now, but clicking on the links to files/directories does not work unless the URL (in the address bar) ends with a forward-slash. If the forward-slash is missing then Servo does not realise it's in a directory and clicking on any of the links causes Servo to replace the current directory name with whatever was clicked. Which obviously means the page will fail to load. Example: Example: So I need to work out how to tell Servo that it's inside the directory even if the URL in the address bar does not end with a forward-slash. |
|
I think the third commit has fixed the problem related to local directory paths not ending with a forward-slash in the address bar. The links in the directory listing now work whether or not the path in the address bar ends with a forward-slash, though I'm wondering whether there's a way to force the address bar to update so that there's no difference at all between visiting a local directory path with or without the forward-slash. |
There was a problem hiding this comment.
Thanks for kicking this off. This should be simplified before it can be merged.
- Directories and files should be listed together in alphabetical order. Make sure that ".." is included.
- Instead of using tables, push a structure of
<div>, which are styled using classes. This will allow more flexible styling simply by modifying the CSS. This structure should be as simple as possible and be valid modern non-quirks HTML5. For example:
<!DOCTYPE html>
<html>
<head>
<link rel="stylesheet" href="">
</head>
<body>
<div class="directory-listing">
<div class="file">
<div class="name">filename.txt</div>
<div class="size">10 KB</div>
<div class="last-modified">01/01/2024</div>
</div>
</div>
</body>
</html>
|
Thank you the review, @mrobinson. I'm going to try to get the CSS into an external file now. But I'm going to leave the markup in the form of a table so that it remains accessible to users who rely on screen readers, and other tools which rely on semantic markup. With the additional If you're certain it should not be a table structure, and you're sure it won't hurt accessibility to move it out of a table, then let me know. But I reckon we can have the best of both by using |
|
FWIW Servo has no accessibility support at the moment so this conversation is highly hypothetical. That said, I'm not sure that a table is really helpful for AT users here. If table display is necessary it can be applied via CSS with |
|
Okay, I have CSS working as an external resources file now. And directories and files are now together, rather than in two groups. And I've added a link to the parent directory (unless the path has no parent). My thinking with regards to tables versus divs is based on advice such as found here. But if Servo is not yet in a place where screen readers can play with it, then I'll change One thing I still haven't found an answer for: how do we format a |
|
Hello, @mrobinson. I've moved the CSS into a file in the resources directory. Replaced table with nested div elements. Added a link up to the parent directory. And I've improved the presentation of file sizes which are too big to be shown in bytes. Still need to work out how to format Can you review the latest revision and let me know how it's all looking at the moment? |
Servo depends both on |
|
@fabricedesre, yes the 0.3.36 version of the time crate will work nicely. (It provides an How can I make use of the 0.3.36 version within just the one module, without breaking things anywhere else? Oh, and I've just spotted that |
|
@mrobinson @fabricedesre The only way I can find to make the 0.3 version of the time crate available is to add something like this to the Cargo.toml file: That way, I can refer to the Does this approach seem acceptable, or do you know of a better way? |
|
@Bobulous yes using a package alias is how this kind of problem is solved. |
|
Thank you, @fabricedesre, I've got the last-modification date printing nicely now. I'm now working on the stylesheet, and on adjusting the directory listing unit test (because last-modification date is going to be a pain to match exactly, assuming the timestamps get adjusted every time someone checks out the project). |
|
@mrobinson I've made numerous changes, including the addition of actual last-modified dates, and more readable file sizes. I suspect you might say you hate the CSS styling I've applied (I was looking at the Servo logo and website when choosing the colours), and may ask for the HTML to be simplified further, but please take a look and let me know what you'd like to see changed. |
|
Maybe instead of querying real fs in unit test, virtual fs can be constructed instead at the start of test. Also having test files in |
|
Ah, I'm presuming the benefit of that would be:
Are there any disadvantages? Assuming not, can you point me to an example of defining a virtual file system for a unit test, @sagudev? |
|
@sagudev, I'm not having any luck finding a viable way of creating a virtual file system which Servo would then be able to read from (nothing compatible with So now I'm thinking of splitting up the functionality into steps:
That way, the unit test can be split into tests for each step, and then the current all-in-one test can be changed so that it just checks that actually asking Servo to visit an existing local directory will cause an HTML response to be returned which does not contain the something-went-wrong message (but does not check the exact content of the HTML). This will make it easy to test the HTML generation step for a variety of scenarios, because the directory summary struct can be easily mocked for each test. It won't be possible to reliably check the directory-to-struct step, though, because we'll have the same problem as before: no reliably stable directory. But if the test points to the servo project root directory, then at least the unit test can assert that a "Cargo.toml" and "Cargo.lock" file must exist, and that sub-directories must exist too. However, if I'm missing an easy way to simply define a virtual directory which can be fed to |
|
Yeah, I looked again and it seems that all require wrapper for fs functions. But there are crates for tmp directories, so those can be used and manually populated at the start of the test. https://docs.rs/tempfile/latest/tempfile/fn.tempdir.html is already in servo's dependency graph. |
|
Okay, I'll use tempfile to create a temporary directory instead of targeting an actual project directory. But it seems that tempfile does not offer a way to adjust the last-modified dates of the temporary files that it creates, so it will still lead to the problem of not being able to check that last-modified datetimes are rendered correctly in HTML. Consequently, I'm trying to write unit tests which check the individual steps (such as render correct HTML from an artificially created directory summary) but I'm having trouble getting the functions to be visible to the unit test file. I was under the impression that |
|
I've removed the actual demo directory, and tempfile is now used to create a temporary directory for the unit tests which need to read a known list of file and subdirectory items. (Still can't set the last-mod dates, though.) The unit tests have been reworked, but I still want to reduce the visibility of the new functions/structs so that they can only be seen from within the methods.rs file and from within the unit tests. I'm hoping someone can review the latest version and tell me what works, what needs adjustment, and what needs to be scrapped. I've spent seven hours on this today, so I think I'd better wait for some feedback before doing any more. |
Signed-off-by: Bobulous <Bobulous@users.noreply.github.com>
…ries. Updated unit tests. Signed-off-by: Bobulous <Bobulous@users.noreply.github.com>
Signed-off-by: Bobulous <Bobulous@users.noreply.github.com>
…ble with nested div elements. Signed-off-by: Bobulous <Bobulous@users.noreply.github.com>
…Added last-modification datetime for files. Signed-off-by: Bobulous <Bobulous@users.noreply.github.com>
…tual demo directory. Signed-off-by: Bobulous <Bobulous@users.noreply.github.com>
Added code to Servo to generate a local directory listing when visiting local directory paths.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors