wxPDF_BORDER_RIGHT doesn't work as expected.

[stkruse]

I think wxPDF_BORDER_RIGHT doesn't work as expected:

Initially, I encountered that I don't get a right border around table cells when using the mark up language with colored rows.

So I played a bit around with the samples and were able to reproduce the issue with sample 5:

Considering in sample5.cpp the funcion FancyTable(), I adjusted it in a way that just the first column should have a right border:

void FancyTable(wxArrayString& header, wxArrayPtrVoid& data)
  {
    // Colors, line width and bold font
    SetFillColour(wxColour(255,0,0));
    SetTextColour(255);
    SetDrawColour(wxColour(128,0,0));
    SetLineWidth(.3);
    SetFont(wxS(""),wxS("B"));
    //Header
    double w[4] = {40,35,40,45};
    size_t i;
    for (i = 0; i < header.GetCount(); i++)
    {
      Cell(w[i],7,header[i],wxPDF_BORDER_FRAME, 0, wxPDF_ALIGN_CENTER, 1);
    }
    Ln();
    // Color and font restoration
    SetFillColour(wxColour(224,235,255));
    SetTextColour(0);
    SetFont(wxS(""));
    // Data
    int fill = 0;
    size_t j;
    for (j = 0; j < data.GetCount(); j++)
    {
      wxArrayString* row = (wxArrayString*) data[j];
      Cell(w[0],6,(*row)[0],wxPDF_BORDER_RIGHT,0,wxPDF_ALIGN_LEFT,fill);
      Cell(w[1],6,(*row)[1], 0,0,wxPDF_ALIGN_LEFT,fill);
      Cell(w[2],6,(*row)[2], 0,0,wxPDF_ALIGN_RIGHT,fill);
      Cell(w[3],6,(*row)[3], 0,0,wxPDF_ALIGN_RIGHT,fill);
      Ln();
      fill = 1 - fill;
    }
    Cell((w[0]+w[1]+w[2]+w[3]),0,wxS(""),wxPDF_BORDER_TOP);
  }

the result is:

which looks wrong.

void FancyTable(wxArrayString& header, wxArrayPtrVoid& data)
  {
    // Colors, line width and bold font
    SetFillColour(wxColour(255,0,0));
    SetTextColour(255);
    SetDrawColour(wxColour(128,0,0));
    SetLineWidth(.3);
    SetFont(wxS(""),wxS("B"));
    //Header
    double w[4] = {40,35,40,45};
    size_t i;
    for (i = 0; i < header.GetCount(); i++)
    {
      Cell(w[i],7,header[i],wxPDF_BORDER_FRAME, 0, wxPDF_ALIGN_CENTER, 1);
    }
    Ln();
    // Color and font restoration
    SetFillColour(wxColour(224,235,255));
    SetTextColour(0);
    SetFont(wxS(""));
    // Data
    int fill = 0;
    size_t j;
    for (j = 0; j < data.GetCount(); j++)
    {
      wxArrayString* row = (wxArrayString*) data[j];
      Cell(w[0],6,(*row)[0],wxPDF_BORDER_LEFT,0,wxPDF_ALIGN_LEFT,fill);
      Cell(w[1],6,(*row)[1], 0,0,wxPDF_ALIGN_LEFT,fill);
      Cell(w[2],6,(*row)[2], 0,0,wxPDF_ALIGN_RIGHT,fill);
      Cell(w[3],6,(*row)[3], 0,0,wxPDF_ALIGN_RIGHT,fill);
      Ln();
      fill = 1 - fill;
    }

    Cell((w[0]+w[1]+w[2]+w[3]),0,wxS(""),wxPDF_BORDER_TOP);
  }

creates a correct table with a left border from the left column. Even a left border from the second column creates a correct border.

The same tests with the function ImprovedTable() produce the correct output. So it seems to be a problem with the combination of filling and right border.

(Tested with wxPDFDoc v1.0.2 and wxWidgets 3.2.1)

[utelle]

I think wxPDF_BORDER_RIGHT doesn't work as expected:

In principle, it does work. However, under certain circumstances one gets unexpected/unwanted results.

Initially, I encountered that I don't get a right border around table cells when using the mark up language with colored rows.

Admittedly, that is a known deficiency. It has to do with the order in which drawing and filling operations are applied. Currently, each table cell is drawn on its own, without taking into account surrounding cells. Let cell A and B be adjacent cells, A on the left, B on the right. First, the right border of A will be drawn, but then thereafter the filling of B will - at least partially - overdraw the right border of A.

Actually, it would be necessary to separate filling and drawing of table cells into 2 steps: first filling all cells, then drawing all borders.

Considering in sample5.cpp the funcion FancyTable(), I adjusted it in a way that just the first column should have a right border: [...]

the result is:

which looks wrong.

Only the cells in every other row are drawn with a fill color. For the unfilled cells the right border is completely visible, for the filled cells the border is partially overdrawn with the fill color.

It will be difficult, if not impossible, to adjust the current table drawing code in such a way that borders and fill colors are always applied correctly.

[...] creates a correct table with a left border from the left column. Even a left border from the second column creates a correct border.

That is to be expected, because the cell left of the current cell will be drawn first, and then the left border of the current cell. That is, the left border is always on top.

The same tests with the function ImprovedTable() produce the correct output. So it seems to be a problem with the combination of filling and right border.

Yes, that is correct. BTW, bottom and top borders will be affected in a similar way.

Due to the Christmas holidays, I will only look into the issue in January. However, I really don't know whether this issue can be fixed without major efforts.

[stkruse]

All right, back from my christmas vaccation, I'd the possibility to rethink the issue.

Probably I was wrong with identifying Cell or DoCell as the problem.
In case the wxpdfdoc user creates his own cells, he is responsible for the size and positioning of them. When they overlap its the users responsibility to know if it's intentionally or not.

Reviewing my origin problem I encountered that I was confused that right borders doesn't appear when using the <table> tag of the markup language.

I would assume, that borders of table cells doesn't overlap.

So, I had a look into wxPdfTable and how the borders are drawn there:

A possible fix would be to include the borders into the cell they belongs to. Therefore we can calculate an offset.

Here, how I adjusted wxPdfTable::WriteRow in pdfxml.cpp after line 605

 // the border should be part of the cell to be not overwritten by an adjacent cell
 // In case of a given border width use half of it as offset for line starting and ending points
 // Use 0.5 as default since the default line width in pdf is 1 
double borderOffset = 0.5;
if(m_borderWidth > 0)
   borderOffset = m_borderWidth * 0.5;
          
if (border & wxPDF_BORDER_LEFT)   m_document->Line(x+borderOffset,   y,   x+borderOffset,   y+h);
if (border & wxPDF_BORDER_TOP)    m_document->Line(x,   y+borderOffset,   x+w, y+borderOffset);
if (border & wxPDF_BORDER_BOTTOM) m_document->Line(x,   y+h-borderOffset, x+w, y+h-borderOffset);
if (border & wxPDF_BORDER_RIGHT)  m_document->Line(x+w-borderOffset, y,   x+w-borderOffset, y+h);

My first tests with that are looking good.

In case this is a solution for you, I attached a path file with my adjustments.
pdfxml.patch

Edit: The solution is not complete. The border could be over text, for example right border over right aligned text. I'll look in the alignment part too.

[utelle]

All right, back from my christmas vaccation, [...]

I haven't looked into the issue in detail yet for the very same reason.

Probably I was wrong with identifying Cell or DoCell as the problem.

Yes, indeed. There is not much that can be done about the Cell method's behaviour. It's the user's responsibility to properly align the cells, when drawing them manually.

In case the wxpdfdoc user creates his own cells, he is responsible for the size and positioning of them. When they overlap its the users responsibility to know if it's intentionally or not.

Exactly.

Reviewing my origin problem I encountered that I was confused that right borders doesn't appear when using the <table> tag of the markup language.

Yes, the table drawing via markup should be adjusted to match expectations. However, this may not be possible to accomplish easily.

I would assume, that borders of table cells doesn't overlap.

IMHO this assumption doesn't hold true, unfortunately - at least not under all circumstances. If we have 2 adjacent cells and we apply a right border to the left cell and a left border to the right cell, the typical user expectation is that the borders are collapsed into a single border, and not to have an actual border between the cells twice as wide as a single cell border.

A possible fix would be to include the borders into the cell they belongs to. Therefore we can calculate an offset.

A simpler and cleaner approach meeting user expectations would probably be to draw a table not in one pass (as is done now), but in 2 or 3 passes:

1. Draw cell backgrounds,
2. Draw cell borders,
3. Draw cell content.

Matters are complicated by the fact that a table can span more than one page. That is, method wxPdfTable::Write and wxPdfTable::WriteRow need to be redesigned.

[stkruse]

My first patch file had the wrong default line offset. Using the last line width would do the trick

double borderOffset = savedLineWidth * 0.5;

instead of

double borderOffset = 0.5;

IMHO this assumption doesn't hold true, unfortunately - at least not under all circumstances. If we have 2 adjacent cells and we apply a right border to the left cell and a left border to the right cell, the typical user expectation is that the borders are collapsed into a single border, and not to have an actual border between the cells twice as wide as a single cell border.

In fact, I wouldn't expect that the borders are collapsed. At least HTML doesn't collapse borders automatically.

See for example:

<table>
    <tr> <td style="border-right: 1px solid">one</td> <td style="border-left: 1px solid">two</td> <td>three</td> </tr>
</table>

For borders at table level, HTML has a special "collapse" style.
And HTML has a default cell spacing from 2. With cell spacing from 0 we get a "double sized" border too.

I think, most people would expect a similar behavior than in HTML. But, of course, changing the behavior of the mark up can break code of people using it, since currently the cells are sharing their border.

I added an adjusted patch file, using the correct offset for an HMTL like result: pdfxml.patch

But, in case you want to have collapsing borders as default, the functions need a redesign. But I won't come with an proposal for such a solution today.

[utelle]

In fact, I wouldn't expect that the borders are collapsed.

Well, expectations may vary, I agree. However, the current behaviour in wxPdfDocument is that cell borders of a table are collapsed. Yes, borders are drawn for each cell separately, but they are drawn on top of each other, resulting in collapsed borders.

At least HTML doesn't collapse borders automatically.

You are right. However, the markup dialect currently supported by wxPdfDocument doesn't support neither style attributes nor CSS. And at the moment there are no plans to extend the markup dialect.

For borders at table level, HTML has a special "collapse" style. And HTML has a default cell spacing from 2. With cell spacing from 0 we get a "double sized" border too.

Currently, the table implementation in the wxPdfDocument markup dialect uses implicitly cellspacing=0. And at the moment it is not possible to set/change the cellspacing.

I think, most people would expect a similar behavior than in HTML.

Maybe. However, that the default value of the HTML attribute border-collapse is separate doesn't necessarily mean that most users really want that behaviour.

But, of course, changing the behavior of the mark up can break code of people using it, since currently the cells are sharing their border.

That's the most important point. Changing the behaviour as you did in your patch proposal will break existing code.

I'm not against adding additional features like separate cell borders, but it is also necessary to support collapsed cell borders as that was the default behaviour up to now.

But, in case you want to have collapsing borders as default, the functions need a redesign. But I won't come with an proposal for such a solution today.

Implementing properly drawn collapsing borders will require some method redesign, but I fear that additionally supporting separate borders and combinations of separate and collapsed borders will cause a major effort.

[stkruse]

I just implemented a version which follows your suggestion, to draw first the backgrounds, then the cell borders and at last the content.

For printing tables over more than one page, it was necessary to precalculate the page header, so that the number of pages can be determined before the writing of the the table starts.

Please feel free to accept/decline/modify pull request #82 which contains my proposal for the discussed changes.

[utelle]

I just implemented a version which follows your suggestion, to draw first the backgrounds, then the cell borders and at last the content.

Great.

For printing tables over more than one page, it was necessary to precalculate the page header, so that the number of pages can be determined before the writing of the the table starts.

Agreed. Up to now that was not necessary, because the y position was implicitly set correctly after each page break.

Please feel free to accept/decline/modify pull request #82 which contains my proposal for the discussed changes.

I will review the PR.

[utelle]

@stkruse kruse Thanks for the effort to fix this long-standing issue (commit fd29112). Closing...