Skip to content

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

Merged
Siedlerchr merged 7 commits into
JabRef:masterfrom
Landi29:fixes-issue-#7415-v2
Feb 15, 2021
Merged

Fixes the issue "Non valid number as font size results in an uncaught exception."#7438
Siedlerchr merged 7 commits into
JabRef:masterfrom
Landi29:fixes-issue-#7415-v2

Conversation

@Landi29

@Landi29 Landi29 commented Feb 8, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

Please fix the checkstyle issues, otherwise lgtm!

@calixtus calixtus left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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

Landi29 commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

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

@calixtus

calixtus commented Feb 9, 2021

Copy link
Copy Markdown
Member

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

@Landi29

Landi29 commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

@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

Landi29 commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

The Database tests fail. Can someone elaborate on this.

@Siedlerchr

Copy link
Copy Markdown
Member

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

@calixtus

calixtus commented Feb 9, 2021

Copy link
Copy Markdown
Member

@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 Landi29 requested a review from calixtus February 10, 2021 08:40
@Landi29

Landi29 commented Feb 10, 2021

Copy link
Copy Markdown
Contributor Author

@calixtus you are welcome.

@Landi29

Landi29 commented Feb 14, 2021

Copy link
Copy Markdown
Contributor Author

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

@tobiasdiez tobiasdiez left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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

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
Copy Markdown
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


// 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
Copy Markdown
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