Fixes issue where type 7 response still queries for search. #30

sloum · 2019-08-29T23:56:40Z

sloum commented

2019-08-29 23:56:40 +00:00

This is an example fix that seems to work. Rather than handling it at the bookmark level (which was the original source of the issue), all responses to a search will be treated as type 1. Further, type one will now parse in a way that plain text lines WILL be rendered. They will be rendered without the indentation found in regular gophermap lines.

This is meant to be one way of handling: #29

I am open to other solutions and this PR exists to get comments on this path forward.

This is an example fix that seems to work. Rather than handling it at the bookmark level (which was the original source of the issue), all responses to a search will be treated as type 1. Further, type one will now parse in a way that plain text lines WILL be rendered. They will be rendered without the indentation found in regular gophermap lines. This is meant to be one way of handling: https://tildegit.org/sloum/bombadillo/issues/29 I am open to other solutions and this PR exists to get comments on this path forward.

sloum added the

bug

label 2019-08-29 23:56:41 +00:00

asdf commented

2019-08-30 07:46:56 +00:00

Hi Sloum,

I wasn't able to figure this out after looking at it again yesterday. These changes explain a lot though so thank you for putting them all through. I appreciate the opportunity to learn.

####Changes to gopher type for completed searches
This works well - bookmarked items from a type 7 link now save as type 1 and display the expected results. Furthermore, existing bookmarks of type 7 load without a prompt.

gopher.go line 100 repeats a line from the MakeURL function that builds the full URL. Should this be a new function like BuildFullURL?

####Changes to document rendering
The changes to gopher/view.go seem to have impacted the way that the help page is rendered. Lines 30 and 35 are rendered as plain text, likely because they are missing the selector, host and port.

Is this appropriate behaviour? I don't know, but present the following:

The document has an error in it, right? I like strict rendering, and this change is a good way to indicate errors.
The intent of lines 30 and 35 is clear even though they may not be correct. Each line begins with a valid file type. lynx and vf1 both render these lines as though they are correct.

Hi Sloum, I wasn't able to figure this out after looking at it again yesterday. These changes explain a lot though so thank you for putting them all through. I appreciate the opportunity to learn. ####Changes to gopher type for completed searches This works well - bookmarked items from a type 7 link now save as type 1 and display the expected results. Furthermore, existing bookmarks of type 7 load without a prompt. `gopher.go` line 100 repeats a line from the `MakeURL` function that builds the full URL. Should this be a new function like `BuildFullURL`? ####Changes to document rendering The changes to `gopher/view.go` seem to have impacted the way that the help page is rendered. Lines 30 and 35 are rendered as plain text, likely because they are missing the selector, host and port. Is this appropriate behaviour? I don't know, but present the following: - The document has an error in it, right? I like strict rendering, and this change is a good way to indicate errors. - The intent of lines 30 and 35 is clear even though they may not be correct. Each line begins with a valid file type. `lynx` and `vf1` both render these lines as though they are correct.

sloum commented

2019-08-30 21:22:52 +00:00

Good catches in both cases. I thought duplicating that code was a little sloppy at the time (but I only had a few minutes to work on it) and now that I think about it I can likely just move the gophertype change into the create url function and just have it done there and eliminate both an additional function as well as that bit of duplication. That said, making BuildFullUrl a receiver would make URLs a bit more flexible when things change... and will make further work easier in the long run probably.

For the rendering, such a good example you found on the help page! I agree that I am not sure what the appropriate behavior is.

I agree that strict rendering guides people to properly created gophermaps. I also like, as you point out, that it doesnt break. The reader can still see they were meant to be item type i and they can still see the content. I think I am inclined to keep the rendering this way.

I coded bombadillo to be pretty strict about gophermaps in general. All lines really should be four fields (tab separated). I have not set up relative linking either, even though some servers support it. I tend to think that servers that extend the spec should spit out a strictly compliant gophermap and that that is what bombadillo renders, but it is a fine line. At least for the moment I think rendering lines without four fields as plain text is good for users and also helps with the fix in question here (type 7 responses).

I will do a few more tweaks to the code tonight or tomorrow before merging. Thanks so much for the input and research! It has been VERY helpful.

Good catches in both cases. I thought duplicating that code was a little sloppy at the time (but I only had a few minutes to work on it) and now that I think about it I can likely just move the gophertype change into the create url function and just have it done there and eliminate both an additional function as well as that bit of duplication. That said, making `BuildFullUrl` a receiver would make URLs a bit more flexible when things change... and will make further work easier in the long run probably. For the rendering, such a good example you found on the help page! I agree that I am not sure what the appropriate behavior is. I agree that strict rendering guides people to properly created gophermaps. I also like, as you point out, that it doesnt break. The reader can still see they were meant to be item type `i` and they can still see the content. I think I am inclined to keep the rendering this way. I coded bombadillo to be pretty strict about gophermaps in general. All lines really should be four fields (tab separated). I have not set up relative linking either, even though some servers support it. I tend to think that servers that extend the spec should spit out a strictly compliant gophermap and that that is what bombadillo renders, but it is a fine line. At least for the moment I think rendering lines without four fields as plain text is good for users and also helps with the fix in question here (type `7` responses). I will do a few more tweaks to the code tonight or tomorrow before merging. Thanks so much for the input and research! It has been VERY helpful.

sloum commented

2019-08-31 02:52:51 +00:00

Ok. I added a simpler fix. The gophertype is changed at the time of url creation rather than the time of request. It should always be right now and no duplication of any code is needed.

asdf commented

2019-08-31 07:53:17 +00:00

Tested. New bookmarks are saved as type 1 and loaded with no prompt. Existing bookmarks of type 7 load without a prompt. So, all good!

Testing was done on veronica 2, contrition and phreader.

The changes look like a good fit for url.go

Tested. New bookmarks are saved as type 1 and loaded with no prompt. Existing bookmarks of type 7 load without a prompt. So, all good! Testing was done on veronica 2, contrition and phreader. The changes look like a good fit for `url.go`

sloum closed this pull request

2019-09-01 02:54:31 +00:00

sloum commented

2019-09-01 02:55:05 +00:00

Thanks asdf! I really appreciate the help. This is emrged in and closes issue Thanks for all the help @asdf! Merging this into master. This PR closes issue #29

Sign in to join this conversation.

No reviewers