Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Column#setWidth is ignored #1265

Closed
Vampire opened this issue Feb 27, 2024 · 3 comments · Fixed by #1270
Closed

Column#setWidth is ignored #1265

Vampire opened this issue Feb 27, 2024 · 3 comments · Fixed by #1270

Comments

@Vampire
Copy link

Vampire commented Feb 27, 2024

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 added a commit to robertpanzer/asciidoctorj that referenced this issue Mar 16, 2024
@robertpanzer
Copy link
Member

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
Copy link
Member

#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
Copy link
Member

I think I would prefer this over doing anything implicitly.

Seems reasonable to me!

robertpanzer added a commit that referenced this issue Mar 31, 2024
* Fixes #1265. Support setting column widths

* Fix test for upstream

* Add test for autowidth columns

* Update changelog
robertpanzer added a commit to robertpanzer/asciidoctorj that referenced this issue Apr 1, 2024
robertpanzer added a commit that referenced this issue Apr 7, 2024
* Fixes #1265. Support setting column widths

* Update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants