fix: fix exception in custom preview layout change #7539

yinpeiqi · 2021-03-15T05:25:23Z

This problem is cause by unexpected exception trigger that listening to value change. In layoutHelper, this function is call by both button click 'save', init loading and value change of text. Actually the last one do not need trigger this exception. So I add a flag from the separation of these three conditions.

Fixses #7526

Change in CHANGELOG.md described in a way that is understandable for the average user (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.

yinpeiqi · 2021-03-15T05:28:51Z

This is just one approach of this issue. Although it works, I still consider that it's a bad implementation and lack of elegance.

calixtus · 2021-03-15T08:49:38Z

Hi @yinpeiqi , thanks for your interest in this issue.
I agree, muting an exception is almost never a good idea.
If I understood the issue description correctly this seems to be a problem about a parser, bound to a listener for changes. Maybe there is a way to suspense parsing the code for about half a second around editing and then restart parsing?

yinpeiqi · 2021-03-15T09:50:17Z

I think suspense parsing the code can't solve this problem. It will cause a time-wait when 'save' is clicked which is unnecessary. Besides, only suspense parsing may not work because users may edit it for a long time.
These three places use the same parser to deal with the format. Most of the uses in the parser are the same but only the exception judgment should be different. Is it possible to replace this parser with two parsers, and used by different places?

calixtus · 2021-03-15T10:34:50Z

I really think we are talking about two different things...

The original issue never said anything about saving. @Siedlerchr suggested, that changes should only be evaluated when pressing save. As I looked into a code a bit, a saw several problems: first if all, we have two different ways to evaluate the code: One thing is syntax highlighting in the GUI, the other is parsing it in logic to create a usable layout. This is needed to create the preview of the changes, if you switch tabs from Edit to Preview.
Right now, both is triggered on every change. And parsing takes a lot of resources.

So my suggestion would be to change the listener on every text change (PreviewTabViewModel line 94-99) to when someone changes to the preview tab and to parse it when the layout is to be saved.

yinpeiqi · 2021-03-15T12:04:24Z

I think I got what you mean. So the listener should not be bind there and we don't need that listener. We should call the setText function only when we click 'preview' or 'save', am I right?

calixtus · 2021-03-15T18:57:15Z

I guess so.

xiongz-c · 2021-03-19T13:24:31Z

I think we can add try-catch when we use parse() in getLayoutFromText(). Then we can handle the StringIndexOutOfBoundsException when the text changed. After discussing with yinpeiqi, we think this way can fix this issue.

calixtus · 2021-03-19T16:35:27Z

No, please don't touch the TextLayout.
Try first the solution about the overused ChangeListener first.

xiongz-c · 2021-03-21T07:29:48Z

You are right. If I use try-catch in getLayoutFromText() just to hide this error from the user, it does not prevent it from happening, and it may affect other places where this method is used. I think the modification logic you mentioned is feasible.

I think we have two different logic to fix this issue: One is to keep the original change monitoring method, but do not call setText when a possible error situation is encountered during monitoring, and the other is to call the setText method when leaving the preview tab or press save button. These two methods have their own problems: the first method will cause the user to discard the changes without prompting the user when an error is encountered. The second method requires additional methods when saving. As I looked into a code a bit, the save of preference is the same method that is reused, which means that we need to call setText once whenever we use the save button. This means that in most cases the save button will execute a meaningless method.

But as a user, I think I would prefer the second way because such repeated calls are not very perceptive, and important error prompts will not be missed. What do you think about it? @calixtus

**update: ** sorry, the second problem does not exist. I can override the method validateSettings() to check the value when saving. So, my way to solve this problem is to add try-catch in the listener to avoid throw error when typing, and add the valid check into the method validateSettings()

…cking when saving

…fix-for-issue-7526

calixtus · 2021-03-21T15:03:08Z

I believe a try-catch clause isn't necessary. The problem is imho the changelistener parsing the whole text after every text change. This isn't necessary at all, but requires a lot of processing power.
The syntax highlighting is controlled directly by the codearea in the richtextfx-package. So we don't have to care about this.
The other thing is the parsing of the text into the TextLayout, which should only happen if one want's to save the changes (and if the parsing fails there really should be an error message) and if you click on the preview tab, so you can live preview your changes before saving.
Like you said, this first problem (or for you the second) can be solved by calling for this check on validateSettings.
For the other problem I believe a changelistener depending on the visible state of the previewTab should be sufficient.

calixtus

Three small problems

calixtus · 2021-03-21T15:04:22Z

src/main/java/org/jabref/gui/preferences/preview/PreviewTabViewModel.java

@@ -304,6 +316,7 @@ public void selectedInChosenDown() {

    public void resetDefaultLayout() {
        PreviewLayout defaultLayout = findLayoutByName("Preview");
+        System.out.println("resetDefaultLayout");


Please use our logger infrastructure.

sorry, just for debug. I will remove it.

calixtus · 2021-03-21T15:04:34Z

src/main/java/org/jabref/gui/preferences/preview/PreviewTabViewModel.java

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

+    public void checkParseWhenSave() {


Why public?

calixtus · 2021-03-21T15:04:56Z

src/main/java/org/jabref/gui/preferences/preview/PreviewTabViewModel.java

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


Code duplication

xiongz-c · 2021-03-24T04:18:02Z

I don't know why I failed some fetcher test. I think I have roughly fixed this issue, what else can I do now? @calixtus

calixtus · 2021-03-24T05:56:22Z

The failing fetcher tests are currently a known problem. You can ignore them.

koppor · 2021-04-12T21:01:10Z

src/main/java/org/jabref/logic/layout/LayoutHelper.java

@@ -260,6 +260,7 @@ private void parseField() throws IOException {
                    }
                    throw new StringIndexOutOfBoundsException(
                            "Backslash parsing error near \'" + lastFive.toString().replace("\n", " ") + '\'');
+


Please do not add an empty line

koppor

This quick fix fixes the issue, but leads to less maintainble code. Please introduce

public Optional<Layout> getLayoutFromText() {

koppor · 2021-04-12T21:09:17Z

src/main/java/org/jabref/gui/preferences/preview/PreviewTabViewModel.java

+            try {
+                layoutParse();
+            } catch (StringIndexOutOfBoundsException e) {
+                // catch this error but not give to user [when typing]


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.

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.

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

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().

@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 koppor#392 for details.

koppor · 2021-06-27T21:02:26Z

Thank you for working on this and the discussion on Optionals vs. Exceptions. I took the freedom to take over. First of all, I created test cases to really debug the cause and be sure that future changes do not break the contracts.

To free your energy for other work, I take the freedom to close this PR.

Follow-up at #7851.

fix: for issue 7526

f796415

calixtus mentioned this pull request Mar 18, 2021

Editing "Custom preview style" triggers exception #7526

Closed

Siedlerchr changed the title ~~fix: for issue 7526~~ fix: fix exception in custom preview layout change Mar 19, 2021

rollback: unset flag setting

08027e6

xiongz-c and others added 4 commits March 21, 2021 16:35

add try-catch to avoid throw error when user typing and add valid che…

7009f35

…cking when saving

Merge branch 'master' into fix-for-issue-7526

cfc074d

code style check

c5af0a1

Merge branch 'fix-for-issue-7526' of github.com:yinpeiqi/jabref into …

24b24d5

…fix-for-issue-7526

calixtus requested changes Mar 21, 2021

View reviewed changes

xiongz-c added 2 commits March 21, 2021 23:14

remove some duplicate code

47352ab

check changlog format

158fda7

koppor reviewed Apr 12, 2021

View reviewed changes

koppor requested changes Apr 12, 2021

View reviewed changes

koppor added the status: changes required Pull requests that are not yet complete label Apr 12, 2021

calixtus mentioned this pull request May 24, 2021

fix backslash parsing error in preview layout #7752

Closed

5 tasks

koppor closed this Jun 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix exception in custom preview layout change #7539

fix: fix exception in custom preview layout change #7539

yinpeiqi commented Mar 15, 2021 •

edited by Siedlerchr

Loading

yinpeiqi commented Mar 15, 2021

calixtus commented Mar 15, 2021

yinpeiqi commented Mar 15, 2021

calixtus commented Mar 15, 2021

yinpeiqi commented Mar 15, 2021

calixtus commented Mar 15, 2021

xiongz-c commented Mar 19, 2021

calixtus commented Mar 19, 2021

xiongz-c commented Mar 21, 2021 •

edited

Loading

calixtus commented Mar 21, 2021

calixtus left a comment

calixtus Mar 21, 2021

xiongz-c Mar 21, 2021

calixtus Mar 21, 2021

calixtus Mar 21, 2021

xiongz-c commented Mar 24, 2021

calixtus commented Mar 24, 2021

koppor Apr 12, 2021

koppor left a comment

koppor Apr 12, 2021

EricLee543 Apr 23, 2021 •

edited

Loading

EricLee543 Apr 24, 2021

koppor May 6, 2021

koppor Jun 27, 2021 •

edited

Loading

koppor commented Jun 27, 2021

fix: fix exception in custom preview layout change #7539

fix: fix exception in custom preview layout change #7539

Conversation

yinpeiqi commented Mar 15, 2021 • edited by Siedlerchr Loading

yinpeiqi commented Mar 15, 2021

calixtus commented Mar 15, 2021

yinpeiqi commented Mar 15, 2021

calixtus commented Mar 15, 2021

yinpeiqi commented Mar 15, 2021

calixtus commented Mar 15, 2021

xiongz-c commented Mar 19, 2021

calixtus commented Mar 19, 2021

xiongz-c commented Mar 21, 2021 • edited Loading

calixtus commented Mar 21, 2021

calixtus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiongz-c commented Mar 24, 2021

calixtus commented Mar 24, 2021

Choose a reason for hiding this comment

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricLee543 Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

koppor commented Jun 27, 2021

yinpeiqi commented Mar 15, 2021 •

edited by Siedlerchr

Loading

xiongz-c commented Mar 21, 2021 •

edited

Loading

EricLee543 Apr 23, 2021 •

edited

Loading

koppor Jun 27, 2021 •

edited

Loading