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

Fix breaking FormsFX changes #39

Merged
merged 4 commits into from
Sep 6, 2018
Merged

Conversation

martinfrancois
Copy link
Contributor

Since FormsFX 1.3.0, because of dlsc-software-consulting-gmbh/FormsFX#28 there is no longer a call to getFields(), which was replaced to a call to getElements() instead. Also, a concept of Elements was introduced, which Field now extends. The missing call caused a breaking change, since in our own implementation of a renderer, we used said call.

This fix is not ideal, since it casts the Elements we get from getElement() to a Field, which means if someone would want to define an Element, it would throw a ClassCastException. However, it is a good tradeoff, since we currently only support Field anyways and changing PreferencesFX to consistently use Element instead is a quite major refactoring, as almost all classes use Field in some form, especially including our custom renderers.

@martinfrancois martinfrancois added the bug Something isn't working label Sep 6, 2018
@martinfrancois martinfrancois self-assigned this Sep 6, 2018
….3.0 to `getElement()`, with a cast to `Field.

Since `Field` is more specific than `Element` and the call to `getElements()` is being cast into `Field`s, passing in an `Element` could cause a ClassCastException, but since we already have the possibility of using a node, this is not an issue.
@dlemmermann
Copy link
Collaborator

But aren't those elements things like additional text for describing a field? I know that @aalmiray added some functionality for this in FormsFX. Was he planning on also using it in PreferencesFX?

@martinfrancois
Copy link
Contributor Author

martinfrancois commented Sep 6, 2018

@dlemmermann could be, but in this case I would suggest to add the Element functionality at a later point to the API, also exposing Element instead of Field for Settings.

@dlemmermann dlemmermann merged commit 34e4d8e into develop Sep 6, 2018
@aalmiray
Copy link
Contributor

aalmiray commented Sep 6, 2018

Indeed. Element is required in order to render additional content related to a Field. This is part of the possible enhancements that can be added to PreferencesFX.

@dlemmermann
Copy link
Collaborator

OK, but this has to be done in a later release then. For now PreferencesFX will only support Fields. I have asked the team to submit an enhancement ticket for the new features.

@martinfrancois martinfrancois deleted the fixBreakingFormsFxChanges branch September 6, 2018 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants