Skip to content

refactor: constraining alignment settings#125

Merged
andreasrosdalw merged 3 commits into
LibrePDF:masterfrom
noavarice:api/to-enums
Dec 1, 2018
Merged

refactor: constraining alignment settings#125
andreasrosdalw merged 3 commits into
LibrePDF:masterfrom
noavarice:api/to-enums

Conversation

@noavarice
Copy link
Copy Markdown
Contributor

@noavarice noavarice commented Dec 1, 2018

Current API for vertical/horizontal alignment setting for table elements is not constrained at all, leading to errors. Also, in absence of documentation, it's not obvious (for me, actually) how to set alignment properly.
PR introduces enum types for available alignment modes as well as its application to table element classes

throw new UnsupportedOperationException(MessageLocalization.getComposedMessage("dimensions.of.a.cell.are.attributed.automagically.see.the.faq"));
}

@Override
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add JavaDoc to this and any other new methods?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not added Javadoc here because it's an implementation of interface method which is documented. I can add @inheritDoc, I guess. What do you think?

}

@Override
public void setHorizontalAlignment(final HorizontalAlignment alignment) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JavaDoc

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

@andreasrosdalw
Copy link
Copy Markdown
Contributor

Thanks for this contribution to the project.

I have just looked briefly at the pull-request so far. Does this change in any possible way change the API, layout or behaviour for existing users of OpenPDF?

If you have time, could you please also add a simple test-case to this pull-request?

@andreasrosdalw
Copy link
Copy Markdown
Contributor

@daviddurand What do you think of this change?

@noavarice
Copy link
Copy Markdown
Contributor Author

noavarice commented Dec 1, 2018

Thanks for this contribution to the project.

I have just looked briefly at the pull-request so far. Does this change in any possible way change the API, layout or behaviour for existing users of OpenPDF?

If you have time, could you please also add a simple test-case to this pull-request?

@andreasrosdal
I understand that introducing changes here without appropriate caution can lead to changed behaviour so I tried not to break this. I only have added a new methods that do same thing, enum values are mapped to existing constants.

@noavarice
Copy link
Copy Markdown
Contributor Author

If you have time, could you please also add a simple test-case to this pull-request?

Guess I can, so I'll try

@noavarice
Copy link
Copy Markdown
Contributor Author

noavarice commented Dec 1, 2018

Added very basic test case that setting alignment through new methods will set an underlying int value. It;s almost useless, but do not know what to test here)
Also, @andreasrosdal, I wanted to ask about license header in new files in case this PR will be merged - what should I paste there?

@andreasrosdalw
Copy link
Copy Markdown
Contributor

Thanks. Can you add license headers like in the other source code files?
LGPL and MPL. Update as you see fit.

@andreasrosdalw andreasrosdalw merged commit 6be8239 into LibrePDF:master Dec 1, 2018
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 this pull request may close these issues.

2 participants