Adds spacing to gemini docs to normalize the formatting with gopher docs #188

Merged
sloum merged 8 commits from gemini-gutter into release2.3.3 1 year ago
sloum commented 1 year ago
Owner

This PR would bring the display of text/gemini documents in line with how gopher displays things:

  • Rows that have a link take up space on the left with the link number
  • Rows that do not have a link get space added to the left equal to the amount added for links

This creates a situation where all content is still left aligned, but the link numbers do not throw things off. This will allow preformatted art that is meant to line up in a certain way with link lines (that reside outside the pre block) to line up as expected.

As an added bonus, I added a little snipped that will make heading lines (#, ##, ###) appear in bold if theme is set to color. Technically it will bold any line that starts with #... which is a naive way of handling it... but it was done on a whim and I think it should be ok for the moment.

I think this improves the visual experience of browsing gemini space at the cost of 5 chars of display... given that tradeoff is already made for gopher I see it as acceptable.

@asdf if you end up with a spare moment I'd love an extra set of eyes on this. The code is a little sloppy and not my best (I dont have a lot of time these days with baby, work, and pandemic... but I thought it would be nice to get this in even if it was a little rushed). Definitely feel free to make changes or point out anything that should be done differently.

Once things with the pandemic wind down I may start thinking seriously about a version 3.0 as this codebase is starting to feel a little unweildy and I can see areas that could be so very much improved and simplified by a full rebuild with better data structures. Anyway, that is a story for another time ;)

This PR would bring the display of text/gemini documents in line with how gopher displays things: - Rows that have a link take up space on the left with the link number - Rows that do not have a link get space added to the left equal to the amount added for links This creates a situation where all content is still left aligned, but the link numbers do not throw things off. This will allow preformatted art that is meant to line up in a certain way with link lines (that reside outside the pre block) to line up as expected. As an added bonus, I added a little snipped that will make heading lines (`#`, `##`, `###`) appear in bold if `theme` is set to `color`. Technically it will bold any line that starts with `#`... which is a naive way of handling it... but it was done on a whim and I think it should be ok for the moment. I think this improves the visual experience of browsing gemini space at the cost of 5 chars of display... given that tradeoff is already made for gopher I see it as acceptable. @asdf if you end up with a spare moment I'd love an extra set of eyes on this. The code is a little sloppy and not my best (I dont have a lot of time these days with baby, work, and pandemic... but I thought it would be nice to get this in even if it was a little rushed). Definitely feel free to make changes or point out anything that should be done differently. Once things with the pandemic wind down I may start thinking seriously about a version 3.0 as this codebase is starting to feel a little unweildy and I can see areas that could be so very much improved and simplified by a full rebuild with better data structures. Anyway, that is a story for another time ;)
sloum added the
enhancement
label 1 year ago
asdf commented 1 year ago
Collaborator

Did some quick tests - compatible with Go 1.11.13, no new linting issues. It seems to be implemented well, but I do have some notes I'll add on the code.

Visually it looks pretty cool and is a good result.

Is it correct that this will only work when viewing content served as a Gemini formatted document? I viewed a few different documents on our server and only the .gmi file displayed in this way, while .txt and .md were not. This is the behaviour I would expect, the same as gopher maps and gopher text documents.

Do you think longer term it might be possible to make the link styling for each protocol more similar?

Great job!

Did some quick tests - compatible with Go 1.11.13, no new linting issues. It seems to be implemented well, but I do have some notes I'll add on the code. Visually it looks pretty cool and is a good result. Is it correct that this will only work when viewing content served as a Gemini formatted document? I viewed a few different documents on our server and only the .gmi file displayed in this way, while .txt and .md were not. This is the behaviour I would expect, the same as gopher maps and gopher text documents. Do you think longer term it might be possible to make the link styling for each protocol more similar? Great job!
asdf reviewed 1 year ago
content.WriteRune('\n')
counter = 0
if p.Location.Mime == "1" {
asdf commented 1 year ago
Poster
Collaborator

Maybe this if and else if block could be changed so we don't run strings.HasSuffix for each line of a document that isn't a gopher map.

The mime type check could be done before the loop in this function, followed by setting the appropriate spacer value for the document type. This block could then become a test of the spacer value itself. A bit like:

func (p *Page) WrapContent(width int, color bool) {
    ...
    spacer := 0
    if p.location.Mime == "1" { //gopher map
        spacer := "           "
    } else if strings.HasSuffix(p.Location.Mime, "gemini") { //gemini document
        spacer := "      "
    }
    ...
    for _, ch := range []rune(p.RawContent) {
        ...
        if spacer > 0 {
            content.WriteString(spacer)
            counter += len(spacer)
        }

I'm not sure if this is easy to understand, so I can make these changes for you to review if it helps.

Maybe this `if` and `else if` block could be changed so we don't run `strings.HasSuffix` for each line of a document that isn't a gopher map. The mime type check could be done before the loop in this function, followed by setting the appropriate spacer value for the document type. This block could then become a test of the spacer value itself. A bit like: ```Go func (p *Page) WrapContent(width int, color bool) { ... spacer := 0 if p.location.Mime == "1" { //gopher map spacer := " " } else if strings.HasSuffix(p.Location.Mime, "gemini") { //gemini document spacer := " " } ... for _, ch := range []rune(p.RawContent) { ... if spacer > 0 { content.WriteString(spacer) counter += len(spacer) } ``` I'm not sure if this is easy to understand, so I can make these changes for you to review if it helps.
sloum commented 1 year ago
Poster
Owner

Setting the spacer to 0 to start with wont work due to typing issues, but aside from that this is a very practical change. I'll do this. 👍

Setting the spacer to `0` to start with wont work due to typing issues, but aside from that this is a very practical change. I'll do this. :+1:
sloum commented 1 year ago
Poster
Owner

I have updated to more or less this exact thing and I just always write the spacer for each new with no link inside the for loop, it is just often set to "" and thus has no effect. This is much improved in a very confusing area of the codebase. Thanks for the suggestion!

I have updated to more or less this exact thing and I just always write the spacer for each new with no link inside the for loop, it is just often set to `""` and thus has no effect. This is _much_ improved in a very confusing area of the codebase. Thanks for the suggestion!
Poster
Owner

Do you think longer term it might be possible to make the link styling for each protocol more similar?

I'm not sure exactly what you mean. Since gemini does not have an equivalent to gophertypes and I dont want to prefetch the mime for each link (so as to not break geminis "one page one request" simplicity), so there would not really be much to put in a (MAP) kind of identifier like gopher. I used different spacer lengths in order to not take up horizontal space in a gemini document needlessly, but I definitely could make them the same spacing.

Let me know if that addressed your question correctly or if you were thinking of it in a different way.

Once we get to 3.0.0 I'd like each protocol module to have its own wrapping function I think... then we can do more individualized complex things.

> Do you think longer term it might be possible to make the link styling for each protocol more similar? I'm not sure exactly what you mean. Since gemini does not have an equivalent to gophertypes and I dont want to prefetch the mime for each link (so as to not break geminis "one page one request" simplicity), so there would not really be much to put in a `(MAP)` kind of identifier like gopher. I used different spacer lengths in order to not take up horizontal space in a gemini document needlessly, but I definitely could make them the same spacing. Let me know if that addressed your question correctly or if you were thinking of it in a different way. Once we get to 3.0.0 I'd like each protocol module to have its own wrapping function I think... then we can do more individualized complex things.
asdf commented 1 year ago
Collaborator

Sorry for not being clearer before. That does answer the question, as I was thinking about alignment. The other thing was the difference in link styling - we use different brackets, sometimes surrounding the gopher type and sometimes around the link number:

(MAP)  1   Gopher Link Text
[1]   Gemini Link Text
HTML [1]Link Text

Some alternatives might be:

MAP [1]   Gopher Link Text
[MAP]  1  Gopher Link Text
(1)   Gemini Link Text

This is really just a nitpick. I like it the way it is, just wanted to run this by you in case you hadn't considered it.

Sorry for not being clearer before. That does answer the question, as I was thinking about alignment. The other thing was the difference in link styling - we use different brackets, sometimes surrounding the gopher type and sometimes around the link number: ```none (MAP) 1 Gopher Link Text [1] Gemini Link Text HTML [1]Link Text ``` Some alternatives might be: ```none MAP [1] Gopher Link Text [MAP] 1 Gopher Link Text (1) Gemini Link Text ``` This is really just a nitpick. I like it the way it is, just wanted to run this by you in case you hadn't considered it.
Poster
Owner

@asdf What do you think of this:

MAP [1]   Gopher Link Text
    [1]   Gemini Link Text

That way the numbers are noted in the same way and can be visually greped more easily accross protocols, but without taking up additional line space for gopher (since the parens got removed from the type).

@asdf What do you think of this: ```text/gemini MAP [1] Gopher Link Text [1] Gemini Link Text ``` That way the numbers are noted in the same way and can be visually greped more easily accross protocols, but without taking up additional line space for gopher (since the parens got removed from the type).
asdf commented 1 year ago
Collaborator

I like your idea. I had a play around with implementing it to see how it would look. Two things I found:

  • Maybe try with parens or no wrapping characters, instead of brackets on the link numbers, to see how you feel about readability.

  • These link numbers are number data in a column, so should probably be right aligned? Gopher currently does this, padding for 2 digits. It works well with pages like gemini://gus.guru:1965/known-hosts

I like your idea. I had a play around with implementing it to see how it would look. Two things I found: - Maybe try with parens or no wrapping characters, instead of brackets on the link numbers, to see how you feel about readability. - These link numbers are number data in a column, so should probably be right aligned? Gopher currently does this, padding for 2 digits. It works well with pages like gemini://gus.guru:1965/known-hosts
Poster
Owner

The problem with padding two digits if the brackets around the number. It looks bad having an empty space inside the brackets. I need to work out an efficient way of adding a space before (or maybe after) the brackets rather than having it inside.

The problem with padding two digits if the brackets around the number. It looks bad having an empty space inside the brackets. I need to work out an efficient way of adding a space _before_ (or maybe after) the brackets rather than having it inside.
asdf commented 1 year ago
Collaborator

Yeah padding bracketed numbers is a problem, although the method you used for Gemini avoids this problem:

linknum := fmt.Sprintf("[%d]", len(links))
splitContent[outputIndex] = fmt.Sprintf("%-5s %s", linknum, decorator)

I don't know enough about performance to say - maybe the double sprintf is another problem, and I can't think of anything better. If it's too hard, don't worry about it.

Yeah padding bracketed numbers is a problem, although the method you used for Gemini avoids this problem: ```Go linknum := fmt.Sprintf("[%d]", len(links)) splitContent[outputIndex] = fmt.Sprintf("%-5s %s", linknum, decorator) ``` I don't know enough about performance to say - maybe the double sprintf is another problem, and I can't think of anything better. If it's too hard, don't worry about it.
Poster
Owner

@asdf I copied over that formatting idea to gopher. Give it a pull/build and let me know what you think. I think it looks good and has a much more unified look between the two main protocols now.

@asdf I copied over that formatting idea to gopher. Give it a pull/build and let me know what you think. I think it looks good and has a much more unified look between the two main protocols now.
asdf commented 1 year ago
Collaborator

Yeah that looks great. Gemini link numbers are left aligned, but it looks like the better choice. Let's push it.

Yeah that looks great. Gemini link numbers are left aligned, but it looks like the better choice. Let's push it.
sloum merged commit d190e0ad00 into release2.3.3 1 year ago
sloum deleted branch gemini-gutter 1 year ago
The pull request has been merged as d190e0ad00.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.