maven-doxia icon indicating copy to clipboard operation
maven-doxia copied to clipboard

Fix snippet reader tab and indent handling

Open Jotschi opened this issue 2 years ago • 15 comments

The PR addresses the following issues:

  • The snippet reader did not correctly apply the indentation for snippet sources which were indented using tabs. The minIndent calculation only checked spaces.

Snippet Source:

[tab][tab]line

Old Behaviour

[tab][tab]line

New Behaviour:

line
  • The snippet reader minIndent calculation was always 0 whenever the snippet contained empty newlines. Those empty lines will now correctly be handled.

Snippet Source:

[space][space]line1
[\n]
[space]line2

Old Behaviour

[space][space]line1
[\n]
[space]line2

New Behaviour:

line1
[\n]
[space]line2

Jotschi avatar Mar 30 '23 09:03 Jotschi

I don't understand this change. It does not retain the original format, what does it address?

michael-o avatar Oct 09 '23 19:10 michael-o

It fixes the indentation behaviour when the source contains tabs. Without the fix the indentation is not correctly handled. Without the fix the source code block has too many tabs and is not rendered in a nice format.

Jotschi avatar Oct 09 '23 19:10 Jotschi

It fixes the indentation behaviour when the source contains tabs. Without the fix the indentation is not correctly handled. Without the fix the source code block has too many tabs and is not rendered in a nice format.

But looking at your examples it does remove tabs as well, no?

michael-o avatar Oct 09 '23 19:10 michael-o

As far as I recall - no - tabs are not removed at all. See first example.

Jotschi avatar Oct 09 '23 19:10 Jotschi

First snippet has two tabs, but expected does not have any. What do I miss?

michael-o avatar Oct 09 '23 19:10 michael-o

The tabs from this example [tab][tab]line will not be removed. Thus the snippet looks ugly. The tabs should be removed.

Jotschi avatar Oct 09 '23 19:10 Jotschi

The tabs from this example [tab][tab]line will not be removed. Thus the snippet looks ugly. The tabs should be removed.

Why? If the snippet is intended that way? How can we know what is right or wrong in snippet content and context?

michael-o avatar Oct 09 '23 19:10 michael-o

@kwin, do you understand this PR?

michael-o avatar Oct 09 '23 19:10 michael-o

The snippet renderer removes unnecessary indentation. It thus moves the code block left. A code example which is highly indented will thus not include the bogus indentation.

{
     {
snipped-start
             { 
                    // your code 
             }
snipped-end
      }
}

should result in

{ 
      // your code 
}

and not

             { 
                    // your code 
             }

Jotschi avatar Oct 09 '23 19:10 Jotschi

But what if

             { 
                    // your code 
             }

is correct indentation?

michael-o avatar Oct 09 '23 19:10 michael-o

The code clearly removes this indentation if it is done by using spaces. Using tabs it is not removed and thus looks broken. I'm okay with it if that is the desired behaviour.

Jotschi avatar Oct 09 '23 19:10 Jotschi

The code clearly removes this indentation if it is done by using spaces. Using tabs it is not removed and thus looks broken. I'm okay with it if that is the desired behaviour.

It is hard to say what is right, but I would say that a snippet should remain as-is and not modified, as ugly as it is.

michael-o avatar Oct 09 '23 19:10 michael-o

I think the indentation is clearly removed in order to prevent horizontal scrollbars for included snippets.

Jotschi avatar Oct 09 '23 19:10 Jotschi

I think the indentation is clearly removed in order to prevent horizontal scrollbars for included snippets.

I'd prefer to leave formatting as-is, but I am open to a snippet option which reverse-indents.

michael-o avatar Oct 09 '23 19:10 michael-o

I'd like to perform a release this weekend. Anyone want to complete the discussion with me?

michael-o avatar Oct 13 '23 07:10 michael-o