Makes local protocol navigable #94

sloum · 2019-11-15T22:55:01Z

sloum commented

2019-11-15 22:55:01 +00:00

I know I said no more features. I was bored on a Friday at work and had already gone over the website release page stuff and updated the makefile for adding desktop files and protocol handlers... so I decided to give this a whirl.

This PR accomplishes the following (all in relation to the local protocol):

No change to displaying files
Directory output now contains links (making it navigable like anything else)
Directory output contains file mode settings (letting you know what you are navigating to)
Directory output is now displayed alphabetically (ascending)

As a part of this I have also tested that error messaging is appropriate when a user tries to open a file they do not have permissions for.

It all works pretty well and actually makes Bombadillo usable as a file browser. Probably wouldn't be my first choice as a daily driver, but it now boasts more than just afterthought file handling.

This is not an urgent item, so if it becomes a choice time-wise to check this out or handle other stuff, I definitely lean toward the other stuff. Though I would put this in the "nice to have" category for 2.0.0 if we can squeeze it in.

I know I said no more features. I was bored on a Friday at work and had already gone over the website release page stuff and updated the makefile for adding desktop files and protocol handlers... so I decided to give this a whirl. This PR accomplishes the following (all in relation to the `local` protocol): - No change to displaying files - Directory output now contains links (making it navigable like anything else) - Directory output contains file mode settings (letting you know what you are navigating to) - Directory output is now displayed alphabetically (ascending) As a part of this I have also tested that error messaging is appropriate when a user tries to open a file they do not have permissions for. It all works pretty well and actually makes Bombadillo usable as a file browser. Probably wouldn't be my first choice as a daily driver, but it now boasts more than just afterthought file handling. This is not an urgent item, so if it becomes a choice time-wise to check this out or handle other stuff, I definitely lean toward the other stuff. Though I would put this in the "nice to have" category for 2.0.0 if we can squeeze it in.

sloum added the

enhancement

non-urgent

local

labels 2019-11-15 22:55:01 +00:00

asdf commented

2019-11-16 20:45:01 +00:00

This is a really good usability improvement.

It handles permissions errors well.

I didn't realise that you can use this to view binary files. That has some weird behaviour, which is expected, but do you think it has a chance to break the display? I assume that filtering of escape codes would prevent this?

This is a really good usability improvement. It handles permissions errors well. I didn't realise that you can use this to view binary files. That has some weird behaviour, which is expected, but do you think it has a chance to break the display? I assume that filtering of escape codes would prevent this?

sloum commented

2019-11-16 21:58:54 +00:00

I'm glad you like it. I think it actually makes it usable as a file browser.

I dont think there should be any issues with escape codes. They should still get filtered out by the line wrapping routine.

I'm glad you like it. I think it actually makes it usable as a file browser. I dont think there should be any issues with escape codes. They should still get filtered out by the line wrapping routine.

asdf commented

2019-11-16 23:04:33 +00:00

Do you know why the terminal bell rings when you browse through binary files? I thought it was because of escape codes but if we are filtering a it must be something else. Not really important though, I'll try to do some research on it.

Do you think you can add an entry for .. so that you can follow a link to go up a level?

Appending / (or a path separator) to the end of directory names would help make it clearer what they are. It might add unnecessary complexity though (I haven't looked at the code yet), and you can already see what is a directory from the left column.

Word wrapping looks good too. It's a really good enhancement overall, functionally I'd say it's ready.

Do you know why the terminal bell rings when you browse through binary files? I thought it was because of escape codes but if we are filtering a it must be something else. Not really important though, I'll try to do some research on it. Do you think you can add an entry for `..` so that you can follow a link to go up a level? Appending `/` (or a path separator) to the end of directory names would help make it clearer what they are. It might add unnecessary complexity though (I haven't looked at the code yet), and you can already see what is a directory from the left column. Word wrapping looks good too. It's a really good enhancement overall, functionally I'd say it's ready.

sloum commented

2019-11-17 01:05:55 +00:00

a is not an VT-100 escape sequence (which is what gets filtered out). It is an ascii character (ascii char 7). In theory I can add that to the filtering if you think that would be a good idea.

I can definitely add the ... That is a great idea that will really help navigability. I can also add the / to directory listings. Both are trivial additions. I'll do them now and let you know once they are in.

`a` is not an VT-100 escape sequence (which is what gets filtered out). It is an ascii character (ascii char 7). In theory I can add that to the filtering if you think that would be a good idea. I can definitely add the `..`. That is a great idea that will really help navigability. I can also add the `/` to directory listings. Both are trivial additions. I'll do them now and let you know once they are in.

sloum commented

2019-11-17 01:40:09 +00:00

Ok. The following has been done:

.. has been added for all directory listings except for the root directory
a has been filtered out for all pages, preventing people from bell spamming (not that I've ever seen anyone do it, but it sounds fun to troll people that way with a phlog entry)
directories now end in /

Ok. The following has been done: - `..` has been added for all directory listings except for the root directory - `a` has been filtered out for _all_ pages, preventing people from bell spamming (not that I've ever seen anyone do it, but it sounds fun to troll people that way with a phlog entry) - directories now end in `/`

asdf approved these changes 2019-11-17 20:32:59 +00:00

asdf left a comment

A couple of items of feedback, but otherwise pretty solid. A good enhancement.

local/local.go Outdated

						
				@ -23,2 +27,3 @@

						fileList, err := file.Readdir(0)

						if err != nil {

							return "", fmt.Errorf("Unable to read from directory: %s", address)

							return "", links, fmt.Errorf("Unable to read from directory: %s", address)

asdf commented

2019-11-17 20:07:20 +00:00

I've found it can be useful to include the error message coming from the FS operation. It will normally indicate why something has happened, and the error message is somewhat readable. There's other instances of this here, is this something you would consider changing?

sloum commented

2019-11-17 22:59:44 +00:00

I agree. The error messaging for this module is generally pretty readable and will be useful to users. I have changed this in the relevant locations in locals.go.

I agree. The error messaging for this module is generally pretty readable and will be useful to users. I have changed this in the relevant locations in `locals.go`.

local/local.go Outdated

						
				@ -28,2 +33,2 @@

						for _, obj := range fileList {

							out.WriteString(obj)

						if address != "/" {

asdf commented

2019-11-17 20:27:28 +00:00

Looking at bash, it includes ".." in the list of files in root, so this distinction may not be needed from a user perspective. The behaviour I see is that following .. from root just lands you back at root. Removing this logic check seems to do the same thing with no error messages, maybe it can be omitted?

If you did want to keep doing this, hardcoding root as "/" may not be as compatible as using filepath.VolumeName (but, I don't really know, it is just the closest thing I could see).

Looking at bash, it includes ".." in the list of files in root, so this distinction may not be needed from a user perspective. The behaviour I see is that following `..` from root just lands you back at root. Removing this logic check seems to do the same thing with no error messages, maybe it can be omitted? If you did want to keep doing this, hardcoding root as `"/"` may not be as compatible as using `filepath.VolumeName` (but, I don't really know, it is just the closest thing I could see).

sloum commented

2019-11-17 23:01:24 +00:00

Yup, a quick test in play.golang.org shows that this works well. I agree ditching that hard-coded base path is a better cross-platform solution. I have removed the if and it just always builds the .. path version. I also added some basic comments to the code in-case it ever needs revisiting.

Yup, a quick test in `play.golang.org` shows that this works well. I agree ditching that hard-coded base path is a better cross-platform solution. I have removed the `if` and it just always builds the `..` path version. I also added some basic comments to the code in-case it ever needs revisiting.

asdf commented

2019-11-17 20:35:45 +00:00

Sorry I wrote "otherwise pretty solid" in the review somewhat automatically, and it doesn't really reflect my feeling on it. It is good solid work, regardless of the items I've called out for review. Well done once again.

sloum commented

2019-11-17 23:05:13 +00:00

Having made the two requested updates (I left comments on both, hopefully you can still see them?), I am merging this in. If anything else comes up with this, let me know. I think it should be good though.

No worries about the language. I know you weren't intending to, as the kids say: "throw shade", lol. Realistically, I am pretty happy with someone calling my code pretty solid, as it isnt always the case. I'll take it! 👍 That said, thank you for the well done :)

Having made the two requested updates (I left comments on both, hopefully you can still see them?), I am merging this in. If anything else comes up with this, let me know. I think it should be good though. No worries about the language. I know you weren't intending to, as the kids say: "throw shade", lol. Realistically, I am pretty happy with someone calling my code _pretty solid_, as it isnt always the case. I'll take it! :+1: That said, thank you for the well done :)

sloum closed this pull request

2019-11-17 23:05:25 +00:00

sloum deleted branch improved-local

2019-11-17 23:06:37 +00:00

Sign in to join this conversation.

No reviewers