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

Fixes the issue "Non valid number as font size results in an uncaught exception." #7438

Merged
merged 7 commits into from
Feb 15, 2021

Conversation

Landi29
Copy link
Contributor

@Landi29 Landi29 commented Feb 8, 2021

Fixes #7415

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@Siedlerchr
Copy link
Member

Please fix the checkstyle issues, otherwise lgtm!

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Please use org.jabref.gui.util.OnlyIntegerFormatter

@Siedlerchr
Copy link
Member

@calixtus The problem is that the OnlyIntegerFormatter returns null for no value and this will throw an exception in the spinner IntegerConverter (was my first attempt either)

@Landi29
Copy link
Contributor Author

Landi29 commented Feb 9, 2021

Just to comment on that @Siedlerchr, the Textformatter<Integer> also returns null on no input, at least when I did a manual test.

@calixtus
Copy link
Member

calixtus commented Feb 9, 2021

Isn't it possible to pass a default value as an argument to the constructor?

@Landi29
Copy link
Contributor Author

Landi29 commented Feb 9, 2021

@calixtus Are you referring to the OnlyIntegerFormatter or the null reference exception? In either case, a default value is passed to the TextFormatter, but a null reference exception can still happen.

@Landi29
Copy link
Contributor Author

Landi29 commented Feb 9, 2021

The Database tests fail. Can someone elaborate on this.

@Siedlerchr
Copy link
Member

The database test sometimes fail for no reason, not related to you changes

@calixtus
Copy link
Member

calixtus commented Feb 9, 2021

@calixtus Are you referring to the OnlyIntegerFormatter or the null reference exception? In either case, a default value is passed to the TextFormatter, but a null reference exception can still happen.

I was referring to the OnlyIntegerFormatter. But I am satisfied. Thank you for your work here.

@Landi29
Copy link
Contributor Author

Landi29 commented Feb 10, 2021

@calixtus you are welcome.

@Landi29
Copy link
Contributor Author

Landi29 commented Feb 14, 2021

@calixtus I think you need to approve before we can merge, since you requested changes.

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks! A few very minor remarks from my side. Apart from this, +1 for merge.

if (Pattern.matches("\\d*", c.getText())) {
return c;
}
c.setText("0");
Copy link
Member

Choose a reason for hiding this comment

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

I think returning null here is slightly better, at least according to the documentation "Returning null rejects the change." https://docs.oracle.com/javase/8/javafx/api/javafx/scene/control/TextFormatter.html#getFilter--

Copy link
Member

Choose a reason for hiding this comment

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

See the comments in this thread. Returning null will produce an exception in the spinner

@@ -27,6 +31,16 @@

private final ControlsFxVisualizer validationVisualizer = new ControlsFxVisualizer();

// The fontSizeFormatter formats the input given to the fontSize spinner so that non valid values cannot be entered.
private TextFormatter<Integer> fontSizeFormatter = new TextFormatter<Integer>(new IntegerStringConverter(), 9,
Copy link
Member

Choose a reason for hiding this comment

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

As this might be handy also in other places, I would propose to extract this to a static method in the a helper class gui.util.TextFormatter

@@ -27,6 +31,16 @@

private final ControlsFxVisualizer validationVisualizer = new ControlsFxVisualizer();

// The fontSizeFormatter formats the input given to the fontSize spinner so that non valid values cannot be entered.
private TextFormatter<Integer> fontSizeFormatter = new TextFormatter<Integer>(new IntegerStringConverter(), 9,
c -> {
Copy link
Member

Choose a reason for hiding this comment

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

please don't use abbreviations as variable names. Here change would be good.

@Siedlerchr Siedlerchr merged commit edcd711 into JabRef:master Feb 15, 2021
Siedlerchr added a commit that referenced this pull request Feb 21, 2021
* upstream/master:
  Bump pascalgn/automerge-action from v0.13.0 to v0.13.1 (#7445)
  Auto-approve depend-a-bot-PRs (#7332)
  Clarify that changelog is user-facing
  Remove unmaintained AUTHORS file
  Fixes the issue "Non valid number as font size results in an uncaught exception." (#7438)
  Zbmath fetcher (#7440)
  Bump me.champeau.gradle.jmh from 0.5.2 to 0.5.3 (#7444)
  Bump styfle/cancel-workflow-action from 0.7.0 to 0.8.0 (#7446)
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.

Non valid number as font size results in an uncaught exception.
4 participants