-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
Discussion on plans for version 2.0 #20
Comments
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. |
@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. 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 :) |
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. Dries |
Hello @dobluth @Kravs and everyone else, I understand you concern, and I'm trying to make the use of the library more userfriendly. 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 : 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 |
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 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 Markus |
In regards to you last question, we're working on better information on how to use it in issue #41. |
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: Besides the several parameters we already talked about, it throws an Investigating the need for the checked exception, it appears the constructor itself does nothing that would justify throwing an Loading fonts can throw an 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: 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 Maybe you can go even further than cell templates and create an interface, say 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 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 |
Hi Markus, these are all good points. 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. |
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! |
Hi, I started thinking about this a little bit and would like some feedback. 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(); //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(); //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(); //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(); //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(); Here's the output for all of the above : |
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.
I like the paradigm of method chaining. Maybe we should ensure to consequently use it, even for
Where does the page come from? Can't we use the
Do we still need to extend the
Side note: maybe we can get rid of the generic type? Will have to check that.
Another side note: It is not neccessary to put
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:
This should not be possible with a table that is separated from the layout.
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?
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()
.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
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! :) |
Thanks @dobluth , again you make excellent points.
That was indeed unnecessary. The PageProvider can take care of that.
Table is an abstract class, and I don't want to break backwards compatibility. What benefit would removing that 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.
Passing
That's a good point, I fixed that.
I'm assuming you mean that Layouters are only associated to the drawing of the table, and forgotten
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.
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. |
Hi @dhorions, thanks for already implementing some of the mentioned points, you are blazing fast :)
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
No, but you can get rid of wrapping it into an table.createRow(new ArrayList<>(Arrays.asList(...))); you can simply use table.createRow(Arrays.asList(...)); , assuming
I am not sure I fully understand this. Are there now two ways of defining a header row?
No, that is not exactly my suggestion. At the moment I see two ways of handling it:
So the
What if the 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 " Thanks again for your ongoing effort, keep going! 👍 |
I checked the generic type on Maybe we also could get rid of And as it is deprecated right now, we could also remove |
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.
Right, that was obvious, thanks.
Yes, there are now two ways. I see how that would be confusing.
Not sure I have a preference, what would be the added value of a
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
Yeah, I guess I'll add that to version 1.4.x as well to avoid that recurring question. |
Should we open a gitter.im channel for this discussion? |
@dhorions |
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?
The text was updated successfully, but these errors were encountered: