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.

Sign up for GitHub

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: fix exception in custom preview layout change #7539

Closed
wants to merge 8 commits into from
Closed

fix: fix exception in custom preview layout change #7539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f796415
fix: for issue 7526
yinpeiqi Mar 15, 2021
08027e6
rollback: unset flag setting
xiongz-c Mar 21, 2021
7009f35
add try-catch to avoid throw error when user typing and add valid che…
xiongz-c Mar 21, 2021
cfc074d
Merge branch 'master' into fix-for-issue-7526
xiongz-c Mar 21, 2021
c5af0a1
code style check
xiongz-c Mar 21, 2021
24b24d5
Merge branch 'fix-for-issue-7526' of github.com:yinpeiqi/jabref into …
xiongz-c Mar 21, 2021
47352ab
remove some duplicate code
xiongz-c Mar 21, 2021
158fda7
check changlog format
xiongz-c Mar 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We fixed an issue where Content selector does not seem to work for custom fields. [#6819](https://github.com/JabRef/jabref/issues/6819)
- We fixed an issue in which a linked online file consisting of a web page was saved as an invalid pdf file upon being downloaded. The user is now notified when downloading a linked file results in an HTML file. [#7452](https://github.com/JabRef/jabref/issues/7452)
- We fixed an issue where opening BibTex file (doubleclick) from Folder with spaces not working. [#6487](https://github.com/JabRef/jabref/issues/6487)
- We fixed an issue where editing "Custom preview style" triggers exception. [#7526](https://github.com/JabRef/jabref/issues/7526)

### Removed

Expand Down
15 changes: 12 additions & 3 deletions src/main/java/org/jabref/gui/preferences/preview/PreviewTabViewModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,10 @@ public PreviewTabViewModel(DialogService dialogService, PreferencesService prefe
initialPreviewPreferences = preferences.getPreviewPreferences();

sourceTextProperty.addListener((observable, oldValue, newValue) -> {
var currentLayout = getCurrentLayout();
if (currentLayout instanceof TextBasedPreviewLayout) {
((TextBasedPreviewLayout) currentLayout).setText(sourceTextProperty.getValue().replace("\n", "__NEWLINE__"));
try {
layoutParse();
} catch (StringIndexOutOfBoundsException e) {
// catch this error but not give to user [when typing]
Copy link
Member

Choose a reason for hiding this comment

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

Catching runtime excpetions is a very bad thing. Where exactly is the StringIndexOutOfBoundsException thrown? Exceptions should be catched as early as possible.

In this case, it is thrown at parse() in LayoutHelper.java line 191.

The real fix is that org.jabref.logic.layout.LayoutHelper#getLayoutFromText should return Optional<Layout> and not throw any exception.

Copy link

@EricLee543 EricLee543 Apr 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hello, sir. I am also following this issue. Do you think this exception StringIndexOutOfBoundsException is unnecessary? This StringIndexOutOfBoundsException will be thrown when '\' is entered(in editing stage).
What if it's in the saving stage? If user click the save button, and there is only a '\', should the exception still be thrown?
If we do not throw any exception, the user may not be aware that there is a wrong '\'. If you insist that throwing no exception is a better choice, I can help you implement it. Don't worry about @yinpeiqi and @zc-BEAR. I have communicated with them. Looking forward to your reply.

Copy link

Choose a reason for hiding this comment

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

After my investigation, Layout could not be null. Therefore LayoutHelper#getLayoutFromText seems not effective. In my opinion, we can delete the code
throw new StringIndexOutOfBoundsException( "Backslash parsing error near \'" + lastFive.toString().replace("\n", " ") + '\''); in LayoutHelper#parseField.
I also found that Layout's constructor will warn "Nested field/group entries are not implemented!" in logger if the user input's invalid. In this case, does the error message need to be presented to the user? @koppor

Copy link
Member

Choose a reason for hiding this comment

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

It's not about Layout being null, its about getting rid of the exception. Using exceptions for control flow is a bad thing (see also Java-by-Comparison). Please refactor LayoutHelper not to throw any exception at parse() or getLayoutFromText().

Copy link
Member

@koppor koppor Jun 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@EricLee543 I am not sure how to present that to the user. In the long run, we should switch to Apache Velocity as this shifts maintenance effort from JabRef to another project. See https://github.com/koppor/jabref/issues/392 for details.

}
});

Expand All @@ -110,6 +111,13 @@ public PreviewTabViewModel(DialogService dialogService, PreferencesService prefe
);
}

private void layoutParse() {
var currentLayout = getCurrentLayout();
if (currentLayout instanceof TextBasedPreviewLayout) {
((TextBasedPreviewLayout) currentLayout).setText(sourceTextProperty.getValue().replace("\n", "__NEWLINE__"));
}
}

public BooleanProperty showAsExtraTabProperty() {
return showAsExtraTab;
}
Expand Down Expand Up @@ -233,6 +241,7 @@ public ValidationStatus chosenListValidationStatus() {

@Override
public boolean validateSettings() {
layoutParse();
ValidationStatus validationStatus = chosenListValidationStatus();
if (!validationStatus.isValid()) {
if (validationStatus.getHighestMessage().isPresent()) {
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/jabref/logic/layout/LayoutHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ private void parseField() throws IOException {
}
throw new StringIndexOutOfBoundsException(
"Backslash parsing error near \'" + lastFive.toString().replace("\n", " ") + '\'');

Copy link
Member

Choose a reason for hiding this comment

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

Please do not add an empty line

}

if ("begin".equalsIgnoreCase(name)) {
Expand Down