asciidoctor / asciidoctorj

:coffee: Java bindings for Asciidoctor. Asciidoctor on the JVM!
http://asciidoctor.org
Apache License 2.0
617 stars 172 forks source link

`Column#setWidth` is ignored #1265

Closed Vampire closed 3 months ago

Vampire commented 4 months ago

When using Column#setWidth on the AST API, this is completely ignored.

And not only when modifying an existing table that was originally constructed from parsed adoc, but also when you build a new table from ground up, for example in a tree processor.

I expected when I set the width of the first column to 1 and the second to 1 too, that it behaves like if I would have that in an adoc table columnspec.

But actually, not matter what you set the width to via AST API, the columns (just tried with two columns) are always treated like first column set to 1 and second set to 0. To get what you want currently, you instead have to set the attribute colpcwidth manually like you want it.

I would expect using setWidth to be effective properly, at least when constructing the table from ground up.

mojavelinux: It might be worth mentioning in the issue the reason the value needs to be mapped. It's because width is an evaluated property and the API needs to perform that work since the assignment comes after the evaluation that's done internally by the parser. In other words, setting width alone has no impact.

So if the parser is doing also other calculations, it might be the same issue.

robertpanzer commented 3 months ago

Before doing anything in the Java space I played with building an extension in Ruby that would accept the widths. I had to do this:

    test 'should create table with width' do
      input = 'table::[]'

      begin
        Asciidoctor::Extensions.register do
          block_macro :table do
            process do |parent, target, args|
              table = Asciidoctor::Table.new(parent, {})
              col1 = Asciidoctor::Table::Column.new(table, 0)
              table.columns << col1
              col2 = Asciidoctor::Table::Column.new(table, 1)
              table.columns << col2
              col1.set_attr 'width', 2
              col2.set_attr 'width', 3
              table.assign_column_widths(width_base=5, autowidth_cols=nil)
              table.rows.body << [
                Asciidoctor::Table::Cell.new(col1, 'first'),
                Asciidoctor::Table::Cell.new(col2, 'second')
              ]
              table.set_attr 'rowcount', table.rows.body.length
              table
            end
          end
        end

        output = convert_string_to_embedded input
        assert_equal output, <<~EOM.chomp
        <table class="tableblock frame-all grid-all stretch">
        <colgroup>
        <col width="40%">
        <col width="60%">
        </colgroup>
        <tbody>
        <tr>
        <td class="tableblock halign-left valign-top"><p class="tableblock">first</p></td>
        <td class="tableblock halign-left valign-top"><p class="tableblock">second</p></td>
        </tr>
        </tbody>
        </table>
        EOM
      ensure
        Asciidoctor::Extensions.unregister_all
      end
    end

(Please excuse if any of my Ruby art is making your eyes bleed)

If we just want to mimic the same API in AsciidoctorJ we could just add a method Table.assignColumnWidths(). This would iterate over all existing columns, and sum up the widths that are positive, and collect the columns with a negative width. Then it would call the Ruby method

table.assign_column_widths(width_base=sum of positive widths, autowidth_cols=[columns with negative width])

I think I would prefer this over doing anything implicitly.

robertpanzer commented 3 months ago

1270 demonstrates this attempt and it seems to do the trick.

        // Create the table
        Table table = createTable(parent)
        table.grid = 'rows'
        table.title = "Github contributors of $target"

         // Create the columns 'Login' and 'Contributions'
         Column avatarColumn = createTableColumn(table, 0)
         avatarColumn.width = 1
         Column loginColumn = createTableColumn(table, 1)
         loginColumn.width = 2
         Column contributionsColumn = createTableColumn(table, 2)
         contributionsColumn.width = 2
         contributionsColumn.horizontalAlignment = Table.HorizontalAlignment.CENTER
         table.columns << avatarColumn
         table.columns << loginColumn
         table.columns << contributionsColumn
         table.assignColumnWidths()
mojavelinux commented 3 months ago

I think I would prefer this over doing anything implicitly.

Seems reasonable to me!