dhorions / boxable

Boxable is a library that can be used to easily create tables in pdf documents.
http://dhorions.github.io/boxable/
Apache License 2.0
334 stars 154 forks source link

Discussion on plans for version 2.0 #20

Open dhasilin opened 8 years ago

dhasilin commented 8 years ago

Hello, I have nice experience using boxable. But sometimes people around are worrying about a bit ugly API. Is there any plans to make in nicer and more usefull?

Or even if there is any roadmaps foe project? Some vision?

dobluth commented 8 years ago

I am also unhappy with certain parts of the API. But changing an API is never an easy thing, because you have to ensure that existing code will continue to work. So the usual way is to mark old parts of the API as deprecated, while introducing new methods to replace the deprecated ones. This process ensures that the users have some time to change their code. However, it will take some time to replace the API that way.

But you always have another solution to hide "ugly" API from your friends and co-workers. You can implement utility methods and classes that wrap around the ugly parts, providing sort of an adapter. As soon as the boxable API changes, you would have to adjust the adapter code, while the users of the adapter will not recognize any change. The downside of this approach is that you often have to write ugly code yourself to implement that adapter and that there is another layer that costs performance (if that's an issue for you). But if you need a nice API right now, there probably is no other way.

However, I cannot answer any question to the roadmap, as I am not the maintainer of boxable. It's a pity, @dhorions has not answered any question for a few weeks now.

dhasilin commented 8 years ago

@dobluth , I agree with you :) API is hard to maintain)

And yes, I think I will create an wrapper API for boxable for our report feature. But it is really sounds strange, I thought that boxable is wrapper by itself, for PDFbox project :)

In any case boxable project is really nice, and already saved me few days of work (!) And I happy that this project is OpenSource and shared here. I hope this project will have future.

So thanks to @dhorions I owe him a beer :)

dhorions commented 8 years ago

Hi,

I am sorry to not have been as responsive as I could have been lately. I've been traveling and have been busy with my day job. I hope to be able to get to all issues and concerns soon.

Dries

dhorions commented 8 years ago

Hello @dobluth @Kravs and everyone else,

I understand you concern, and I'm trying to make the use of the library more userfriendly. One example of that is the ability to create tables from csv data in a few lines of code. This is currently being tested and will probably be released soon. It's described, with some examples in #40 , and is accessible in this branch.

In it's most basic form, it can be used like this :

BaseTable dataTable = new BaseTable(yStart, yStartNewPage, bottomMargin, tableWidth, margin, doc, page, true,true);
CSVTable t = new CSVTable(dataTable, page);
t.addCsvToTable(data, CSVTable.HASHEADER, ';');
dataTable.draw();

The output of that will look like this : image

CSVexampleLandscape.pdf

Let me know if that adresses some of your needs, and what other ideas you have that would make boxable more useful for you.

Dries

dobluth commented 8 years ago

Hi everyone,

as mentioned in #40, I like the new feature :)

I especially like the idea of cell templates, as it adresses one of the API flaws: Creating the same cell again and again, varying only in the data part. That said, I think it would be helpful to separate the data and the layout part a little more.

This can also be seen when creating a BaseTable:

BaseTable dataTable = new BaseTable(yStart, yStartNewPage, bottomMargin, tableWidth, margin, doc, page, true,true);

Some questions arise:

In my opinion these parameters are not needed for creating a table, but for drawing it, because that is the only place where data and layout meet.

This is just one example. It would need more time to systematically gather all API flaws and come up with a concept, but the one question regarding API is: what does the usage code look like?

Boxable is a great library and I would appreciate it, if we could find and implement a better API and add new features, like you CsvTable. Again, good work, Dries! :)

Markus

dhorions commented 8 years ago

In regards to you last question, we're working on better information on how to use it in issue #41. I also see your point about separating layout and data. I'll think about that, and am also interested in all other flaws or opportunities for improvement.

dobluth commented 8 years ago

A good idea to make a documentation!

Unfortunately, that is not exactly what I meant. Sorry, I should have made my statement clearer. The question is: do you like the way the usage code looks like? What shall it look like?

The Table class

Okay, let's take a look at the constructor again: public BaseTable(float yStart, float yStartNewPage, float bottomMargin, float width, float margin, PDDocument document, PDPage currentPage, boolean drawLines, boolean drawContent) throws IOException

Besides the several parameters we already talked about, it throws an IOException. I don't like constructors that throw checked exceptions, because it forces me to react somehow, usually by catching it. And just for creating an object, a try-catch-block seems too heavy.

Investigating the need for the checked exception, it appears the constructor itself does nothing that would justify throwing an IOException. It calls loadFonts(), which throws the exception, but does nothing in the implementation of BaseTable.

Loading fonts can throw an IOException, but the loadFonts() method is meant to be overwritten in a subclass, and then I ask myself: why would I load fonts per table? This is usually a process that is done once when creating a PDF.

If the table just holds data, there is no need to extend it in the first place. I could just have a constructor (maybe even parameter-less), that throws no exceptions. Constructing a table should be no more than: Table table = new Table();

The Cell class

There are several issues regarding the cell. First of all, I cannot modify the alignment. This is not too bad, but can turn out annoying.

Another issue is setting the layout related stuff on each cell. Using a template like you did in the CSV branch, softens the pain. It offers templates for the most common cases. But what if I want to have a different background color each third row? Or in each cell that contains a certain word (like "beer" in the test case Sample1())?

Maybe you can go even further than cell templates and create an interface, say CellLayouter:

public interface CellLayouter<T extends PDPage> {
    public void layout(Cell<T> cell, int rowIndex);
}

This is by far not complete, but I want to show a direction. Let's imagine, that each cell gets styled using a CellLayouter. The templates could easily be implemented as CellLayouter, so the user still has access to the common cases, like for example:

public class OddEvenCellLayouter<T extends PDPage> implements CellLayouter<T> {
  @Override
  public void layout(Cell<T> cell, int rowIndex) {
    if (rowIndex % 2 == 0) {
      // set layout for even row
      ...
    } else {
      // set layout for odd row
      ...
    }
  }
}

I hope the idea gets clear, but don't hesitate to ask.

These are still not all API flaws, I will add more if I find the time.

I think by following the maxim to separate layout and data and offer useful defaults we can provide a better API.

Markus

dhorions commented 8 years ago

Hi Markus,

these are all good points. My plan currently is to release version 1.4 so we have all the nice things that were added since the release of 1.3 in the main branch. This addresses already a few of the issues you mention ( Cell now has a few new methods : copyCellStyle, setAlign and setValign ).

When that is released, I would like to work on a new version that will really separate out the creation of the table from any pdf drawing specific items. This should hopefully make the Table and BaseTable classes independant of the pdf document or page. Keep your ideas and comments coming!

ghost commented 8 years ago

I am really happy to find this plugin! With the new PDFBox 2.0 update; this can have serious traction. I will follow closely. Thanks a lot, and keep it up!

dhorions commented 8 years ago

Hi,

I started thinking about this a little bit and would like some feedback. I tried to ensure separation of concerns, and implemented styling and an extensible way of layouting tables.

Here are some usage examples :

Example where the column width of the headers is set manually.


        PDPage p = addNewPage();
        BaseTable table = new BaseTable();
        Row<PDPage> headerRow = table.createRow();
        headerRow.createCell(100, "Test 1 - Print table multiple times to document");
        table.addHeaderRow(headerRow);
        headerRow = table.createRow(
                new ArrayList<>(
                        Arrays.asList(
                                new Cell(10, "Column 1"),
                                new Cell(10, "Column 2"),
                                new Cell(80, "Column 3")
                        )));

        table.addHeaderRow(headerRow);
        //Non header rows will inherit their width from the last header row.
        //Any arbitrary collection can be used, the String representation will be used.
        for (int i = 1; i <= 3; i++) {
            Row<PDPage> row = table.createRow(
                    new ArrayList<>(
                            Arrays.asList(
                                    "Row " + i + " col 1",
                                    "Row " + i + " col 2",
                                    "Row " + i + " col 3")
                    ));
        }
        //Add 1 row with only 2 columns and a differently Styled cells
        table.createRow(
                new ArrayList<>(
                        Arrays.asList(
                                new Cell(80, "Wider Column")
                                .withFont(PDType1Font.TIMES_BOLD)
                                .withFillColor(Color.LIGHT_GRAY),
                                new Cell(20, "Value")
                                .withFont(PDType1Font.TIMES_BOLD)
                                .withFillColor(Color.red)
                                .withTextColor(Color.WHITE)
                        )));
        //Assign table to pageProvider
        DefaultPageProvider provider = new DefaultPageProvider(doc, p.getMediaBox());
        table.setPage(provider);
        try {
            table.draw();
            //Draw it a second time
            table.getRows().get(0).getCells().get(0).setText("Test 1 -  Same table second time on same page");
            table.draw();
            //Use the same table, but with different title, of a different size page (A4 landscape)
            provider.setSize(new PDRectangle(PDRectangle.A4.getHeight(), PDRectangle.A4.getWidth()));
            table.pageBreak();
            table.getRows().get(0).getCells().get(0).setText("Test 1 -  Same table third time, on a different size page");
            //Draw it a third time on the next page
            table.draw();

        }
        catch (IOException ex) {
            //Writing to a pdf page can always return a IOException because of
            //https://pdfbox.apache.org/docs/2.0.0/javadocs/org/apache/pdfbox/pdmodel/PDPageContentStream.html#PDPageContentStream(org.apache.pdfbox.pdmodel.PDDocument,%20org.apache.pdfbox.pdmodel.PDPage,%20org.apache.pdfbox.pdmodel.PDPageContentStream.AppendMode,%20boolean,%20boolean)
            Logger.getLogger(APITestv2_0.class.getName()).log(Level.SEVERE, null, ex);
        }

Example where the column width of the headers is proportionally determined based on the size of the text of the last Header row


        PDPage p = addNewPage();
        BaseTable table = new BaseTable();
        //The Header Row, width % is mandatory for this row
        Row<PDPage> headerRow = table.createRow();
        headerRow.createCell(100, "Test 3 - Auto Calculated Column Width");
        table.addHeaderRow(headerRow);
        //No widths are passed for the columns, so they should be auto calcilated
        headerRow = table.createRow(
                new ArrayList<>(
                        Arrays.asList(
                                "Column 1 is the widest column of them all",
                                "Column 2 is narrower",
                                "Column 3 narrowest"
                        )), Row.HEADER);

        //Non header rows will inherit their width from the last header row.
        //Any arbitrary collection can be used, the String representation will be used.
        for (int i = 1; i <= 3; i++) {
            Row<PDPage> row = table.createRow(
                    new ArrayList<>(
                            Arrays.asList(
                                    "Row " + i + " col 1",
                                    "Row " + i + " col 2",
                                    "Row " + i + " col 3<br/>Only the last header column determines the actual width of the columns, the rest will be wrapped as needed.")
                    ));
        }

        //Assign table to pageProvider
        DefaultPageProvider provider = new DefaultPageProvider(doc, p.getMediaBox());
        table.setPage(provider);
        try {
            table.draw();
        }
        catch (IOException ex) {
            //Writing to a pdf page can always return a IOException because of
            //https://pdfbox.apache.org/docs/2.0.0/javadocs/org/apache/pdfbox/pdmodel/PDPageContentStream.html#PDPageContentStream(org.apache.pdfbox.pdmodel.PDDocument,%20org.apache.pdfbox.pdmodel.PDPage,%20org.apache.pdfbox.pdmodel.PDPageContentStream.AppendMode,%20boolean,%20boolean)
            Logger.getLogger(APITestv2_0.class.getName()).log(Level.SEVERE, null, ex);
        }

Example where layout's and styles are applied

//Draw table once with default Style
table.getRows().get(0).getCells().get(0).setText("Test 4 -  Default Style");
table.getLayouters().add(new DefaultCellLayouter(Styles.DEFAULT));
table.draw();

image

//Green Style with Zebra Layouter
table.getRows().get(0).getCells().get(0).setText("Test 4 -  Green Style with Zebra Layouter");
table.getLayouters().clear();
table.getLayouters().add(new DefaultCellLayouter(Styles.GREEN));
table.getLayouters().add(new ZebraCellLayouter(Styles.GREEN));
table.draw();

image

//Candy Style with Vertical Zebra Layouter
table.getRows().get(0).getCells().get(0).setText("Test 4 -  Candy Style with Vertical Zebra Layouter");
table.getLayouters().clear();
table.getLayouters().add(new DefaultCellLayouter(Styles.CANDY));
table.getLayouters().add(new VerticalZebraCellLayouter(Styles.CANDY));
table.draw();

image

//Default Style with  Zebra Layouter and Matrix Layouter
table.getRows().get(0).getCells().get(0).setText("Test 4 -  Default Style with Zebra Layouter and Matrix Layouter and centered headers");
table.getLayouters().clear();
table.getLayouters().add(new DefaultCellLayouter(Styles.DEFAULT));
table.getLayouters().add(new ZebraCellLayouter(Styles.DEFAULT));
table.getLayouters().add(new MatrixCellLayouter(Styles.DEFAULT));
table.draw();

image

//Custom style
table.getRows().get(0).getCells().get(0).setText("Test 4 -  Custom Style");
DefaultStyle customStyle = new DefaultStyle()
                    .withBorder(new LineStyle(Color.ORANGE, (float) 0.5))
                    .withFont(PDType1Font.COURIER)
                    .withAlignAccent2(HorizontalAlignment.CENTER);
table.getLayouters().clear();
table.getLayouters().add(new DefaultCellLayouter(customStyle));
table.draw();

image

Here's the output for all of the above : APIv2.0.pdf

dobluth commented 8 years ago

First of all: very good and impressive work, Dries! :)

Please apologize, I don't have much time, so I have to be brief. I will just go top down and write everything that comes into my mind.

Cell style properties are applied through method chaining. ( new Cell("content").withFontSize(10).withFillColor(Color.RED) ....

I like the paradigm of method chaining. Maybe we should ensure to consequently use it, even for Table etc.?

PDPage p = addNewPage();

Where does the page come from? Can't we use the PageProvider?

BaseTable table = new BaseTable();

Do we still need to extend the Table? I guess, Table table = new Table(); can be sufficient ;)

Row<PDPage> headerRow = table.createRow();

Side note: maybe we can get rid of the generic type? Will have to check that.

headerRow = table.createRow(new ArrayList<>(...));

Another side note: It is not neccessary to put Arrays.asList() into a List, because it already is one.

table.setPage(provider);

This means, that the table still holds data which is only needed for drawing. Is there a certain reason for not passing it as argument in the draw method: table.draw(provider);? Or maybe we can find another solution.

table.pageBreak();

This should not be possible with a table that is separated from the layout.

headerRow = table.createRow(..., Row.HEADER);

I see you are using another way to create a header row in the second example. I think both ways are valid. Maybe we can even offer both ways?

table.getLayouters().clear(); table.getLayouters().add(new DefaultCellLayouter(Styles.GREEN)); table.getLayouters().add(new ZebraCellLayouter(Styles.GREEN));

Sorry, I had no time to look into the code, but that seems like you would expose a list of layouters. I think we should hide that implementation detail and instead offer methods like table.clearLayouters() and table.addLayouter(). This would also allow for method chaining:

table.clearLayouters()
  .addLayouter(new DefaultCellLayouter(Styles.GREEN))
  .addLayouter(new ZebraCellLayouter(Styles.GREEN))
  .draw();

Again, we should ask ourselves, whether or not we want the table to save layouters. I am not sure about this, because otherwise we would probably need a class like TableLayout, which contains all layout related stuff. But we should avoid adding more classes that a user is forced to use.

new DefaultStyle()..withAlignAccent2(HorizontalAlignment.CENTER)

What does this method do? Only style the second row? This does not get clear by reading the method name, but I assume it by looking at the resulting table :)

The predefined and customizable styles are fantastic! Definitely a huge step in the right direction! :)

dhorions commented 8 years ago

Thanks @dobluth ,

again you make excellent points.

PDPage p = addNewPage();
table.pageBreak();

That was indeed unnecessary. The PageProvider can take care of that.

DefaultPageProvider provider = new DefaultPageProvider(doc, PDRectangle.A4);
provider.nextPage();
table.draw(provider);

Do we still need to extend the Table? I guess, Table table = new Table(); can be sufficient ;) Side note: maybe we can get rid of the generic type? Will have to check that.

Table is an abstract class, and I don't want to break backwards compatibility. What benefit would removing that have for the user?

Another side note: It is not neccessary to put Arrays.asList() into a List, because it already is one.

Can you create an array inline without using Arrays.asList()? If so, please tell me how, I only use this to keep the code in examples/tests as simple as possible.

headerRow = table.createRow(..., Row.HEADER);

I see you are using another way to create a header row in the second example. I think both ways are valid. Maybe we can even offer both ways?

Passing row.HEADER to createRow() marks a row as a header row before its cells are created. That is necessary when you don't pass width percentages to the cell, because in case no widths are passed, the width are auto calculated based on the amount of text in the last header row. For the moment I don't see another solution.

table.getLayouters().clear(); table.getLayouters().add(new DefaultCellLayouter(Styles.GREEN)); table.getLayouters().add(new ZebraCellLayouter(Styles.GREEN));

That's a good point, I fixed that.

Again, we should ask ourselves, whether or not we want the table to save layouters.

I'm assuming you mean that Layouters are only associated to the drawing of the table, and forgotten afterwards. What value would that add? If you want to create a new table, you can create a new table instance and draw on the same PageProvider. All it would change in my opinion is that you would make it harder to draw the same table to multiple documents/pages as you'd have to apply the layouters again after every drawing.

new DefaultStyle()..withAlignAccent2(HorizontalAlignment.CENTER) What does this method do? Only style the second row? This does not get clear by reading the method name, but I assume it by looking at the resulting table :)

The approach I used is this, a Style is independant of a Layouter.

A style is just an object that basically has the following attributes :

Layouters can use these attributes of the Styles as they wish. The MatrixLayouter for example, will use accentColors2 to highlight the cells where x = y.

if ((cell.getRowIndex() - lastHeaderRowIndex) == cell.getColumnIndex()) {
            cell.withFillColor(style.fillcolorAccent2)
                    .withTextColor(style.textcolorAccent2)
                    .withAlign(style.alignAccent2)
                    .withValign(style.valignAccent2);
            }

My first thought was to use attributes like "headerColor", "evenColor" etc. in the Style class, but that would make the layouters less flexible. I'm open to better ideas though :-)

Thanks again or your feedback, looking forward to hearing your thoughts on my response.

dobluth commented 8 years ago

Hi @dhorions,

thanks for already implementing some of the mentioned points, you are blazing fast :)

Table is an abstract class, and I don't want to break backwards compatibility. What benefit would removing that have for the user?

I understand the point of not breaking backwards compatibility, but following the rules of semantic versioning (which I suggest), we can introduce changes in the API that are not backwards compatible, if the major version increments, which this discussion is about, I guess ;) So we would have the possibility to introduce changes that break backwards compatibility. And what's the point in extending the Table class if not adding any functionality? Or in your words: what benefit would keeping it have for the user? ;)

Can you create an array inline without using Arrays.asList()? If so, please tell me how, I only use this to keep the code in examples/tests as simple as possible.

No, but you can get rid of wrapping it into an ArrayList, because the return value of Arrays.asList() already implements the List interface, so instead of

table.createRow(new ArrayList<>(Arrays.asList(...)));

you can simply use

table.createRow(Arrays.asList(...));

, assuming Table.createRow() takes a List.

Passing row.HEADER to createRow() marks a row as a header row before its cells are created.

I am not sure I fully understand this. Are there now two ways of defining a header row?

  1. headerRow = table.createRow(..., Row.HEADER);
  2. headerRow = table.createRow(); table.addHeaderRow(headerRow); If so, do they behave differently? This would be very confusing.

I'm assuming you mean that Layouters are only associated to the drawing of the table, and forgotten afterwards.

No, that is not exactly my suggestion. At the moment I see two ways of handling it:

  1. The current way, saving the layouters in the Table itself.
  2. A separate class, let's say TableLayout, which holds the layouters and maybe other layout-related objects. Drawing a table would require passing an instance of TableLayout (if not using a default?), so different tables can use the same layout. Do you see other options? Which way do you prefer?

So the Style class is a huge data container, home to seven different fill colors, text colors and alignments. There are potential problems:

What if the Style class had no different accents, but only one? The MatrixCellLayouter does need only one, doesn't it? And if you need two different styles, like in ZebraCellLayouter, can't we just pass two Style objects to the constructor, like public ZebraCellLayouter(DefaultStyle oddStyle, DefaultStyle evenStyle)?


Many people seem to have problems fetching the bottom position of the table after drawing it. That's probably also something we should address. The simplest solution could be to offer a getter for that. Using a "PageCursor" could also be a solution, but might be overkill at the moment ;)

Thanks again for your ongoing effort, keep going! :+1:

dobluth commented 8 years ago

I checked the generic type on Table, Row, Cell and PageProvider. In my opinion, those aren't necessary, because we always work with PDPage. If someone really wants to extend PDPage, he would have to implement a PageProvider. This is pretty rare, I guess.

Maybe we also could get rid of AbstractPageTemplate. The only purpose of this class is to offer helper methods to sub classes, which can also be done by PDStreamUtils. In conjunction with that, AbstractTemplatedTable will lose all rights to exist ;)

And as it is deprecated right now, we could also remove BoxableUtils.

dhorions commented 8 years ago

Since we are talking about a major new version here, we could indeed give up on backwards compatibility. I'm not sure I'm comfortable with that idea yet, so I'll let that sink in for a while.

you can simply use

table.createRow(Arrays.asList(...));

Right, that was obvious, thanks.

I am not sure I fully understand this. Are there now two ways of defining a header row?

  1. headerRow = table.createRow(..., Row.HEADER);
  2. headerRow = table.createRow(); table.addHeaderRow(headerRow); If so, do they behave differently? This would be very confusing.

Yes, there are now two ways. I see how that would be confusing.
I'm not sure I know how to resolve it yet. I want it to be possible to not have to worry about column widths and have the api take care of that, but that means that we need to calculate the widths based on something. Currently that is done proportionally based on the text of the last header column. Perhaps I need to rethink when this is calculated alltogether, currently it's done when rows are created, it may be better to just only do that on drawing, I'll try to figure that out.

At the moment I see two ways of handling it:

  1. The current way, saving the layouters in the Table itself.
  2. A separate class, let's say TableLayout, which holds the layouters and maybe other layout-related objects. Drawing a table would require passing an instance of TableLayout (if not using a default?), so different tables can use the same layout. Do you see other options? Which way do you prefer?

Not sure I have a preference, what would be the added value of a TableLayout-class?

So the Style class is a huge data container, home to seven different fill colors, text colors and alignments. There are potential problems:

  • In most cases, you won't need seven different colors and alignments.
  • If you need more, you have a hard time.
  • Although having seven different colors and alignments, I cannot have different fonts. Is there a reason for restricting style accents to only colors and alignments?
  • It is very confusing, because the different styles are only distinguished by numbers, with varying meanings in different layouter implementations. What if the Style class had no different accents, but only one? The MatrixCellLayouter does need only one, doesn't it? And if you need two different styles, like in ZebraCellLayouter, can't we just pass two Style objects to the constructor, like public ZebraCellLayouter(DefaultStyle oddStyle, DefaultStyle evenStyle)?

I see your point, but that would mean the user needs to know more about the Layouters than I want to. They'd need to know how many layouts a style needs, and which one has what purpose. But it more clear than accentColor6 I guess.

Many people seem to have problems fetching the bottom position of the table after drawing it.

Yeah, I guess I'll add that to version 1.4.x as well to avoid that recurring question.

dhorions commented 8 years ago

Should we open a gitter.im channel for this discussion?

Frulenzo commented 8 years ago

@dhorions This is very good idea :) Maybe you can open a repository chat on gitter.im and drop us invites for more fluid conversations