Storing selected preview throws java.lang.IllegalArgumentException: Value too long:

[Siedlerchr]

Current dev version

1. Go to Preferences -> Entry Preview
2. Search American Psychological Association...
3. Move the style to the right
4. Select it or move it up to the top
5. Save preferences:
6. Exception occurs

The problem is that the maximum allowed value for the Preferences Value is:
8192 characters (MAX_VALUE_LENGTH)

Solution: I think the solution would be to store the name of the style

20:45:28.751 [JavaFX Application Thread] ERROR org.jabref.FallbackExceptionHandler - Uncaught exception occurred in Thread[JavaFX Application Thread,5,main]
java.lang.IllegalArgumentException: Value too long: <?xml version="1.0" encoding="utf-8"?> (omitted here) 
	at java.util.prefs.AbstractPreferences.put(AbstractPreferences.java:253) ~[?:?]
	at org.jabref.preferences.JabRefPreferences.put(JabRefPreferences.java:937) ~[main/:?]
	at org.jabref.preferences.JabRefPreferences.storePreviewPreferences(JabRefPreferences.java:1441) ~[main/:?]
	at org.jabref.gui.preferences.PreviewTabViewModel.storeSettings(PreviewTabViewModel.java:203) ~[main/:?]
	at org.jabref.gui.preferences.AbstractPreferenceTabView.storeSettings(AbstractPreferenceTabView.java:30) ~[main/:?]

[leitianjian]

Hi, we want to work on this bug, but it seems difficult to fix it.

Firstly, we have reproduced the bug successfully.
Then, thanks for your tips, we successfully understood the source of the bug

Secondly, we tried to solve it by only store the name instead of style in method JabRefPreferences.storePreviewPreferences(PreviewPreferences previewPreferences)

1. we add a field called previewStyleName in class PreviewPreferences
2. modify the method JabRefPreferences.storePreviewPreferences(PreviewPreferences previewPreferences) to put PREVIEW_STYLE_NAME attribute into preferences
3. try to modify method JabRefPreferences.getPreviewPreferences() to search style by name.
4. we found a method PreviewTabViewModel.findLayoutByName(String name) which can convert the name to style.
5. We try to transfer the field PreviewTabViewModel.availableListProperty,which is used to convert name to style, to the JabRefPreferences.
6. point 5 is a silly idea, because the PreviewTabViewModel is relied on singlton JabRefPreferences. If we pass the PreviewTabViewModel.availableListProperty to the JabRefPreferences can lead to a big problem.
7. The solution we think now is to construct the availableListProperty in the JabRefPreferences, but this solution is not very suitable we thought, because the availableListProperty is not suitable to be included in Preferences, which store setting only.

As a result, we are stucking at this bug without a good solution.
We sincerely waiting for your ideas. It is hard for us to solve this bugs without more info.

It is OK for late reply, we might working on other works recently, please forgive our late reply.
It is OK if you don't think we could working on this bug, we will try something else easier.
In advance, many thanks for your any instructions/tips.

[Siedlerchr]

Hi,
thanks for investigating this and outlining the problems.
I looked into it based on your approach and also for the moment have no idea how to exactly solve this. Mabye @calixtus has an idea. He recently worked a lot with that preferences stuff.

Feel free to tackle another bug in the meantime.

[calixtus]

I fear the source of the bug is somewhat different...

Can you please check if the problem persists if you follow these steps?

1. ...
2. Move the style American Psychological Association ... style to the top
3. Click on the "Preview" style.
4. Click on the "Edit" tab

The name of the syle in the editor tab is now problaby IEEE (in default config). But it shouldn't be.

If you click save, the bug appears.

If you click on the refresh icon in the upper left of the editor ("Reset to default style") the editor contents should be completly different. Saving now should work.

[calixtus]

The text of preview layout always becomes the text of the last CitationStyleLayout in the list, if another style is pushed upwards in the list around the 2nd last one.
The PreviewCycle basically is a list of preview styles, which can be distinguished in CitationStylePreviewLayouts and exactly one TextBasedPreviewLayout. It's all a bit hacky. The CitationStylePreviewLayouts are loaded dynamically, the TextBasedCitationLayout needs to be stored in the preferences to stay persistend. This is the secondary cause of the bug.
So therefor I think the problem is somewhere in the selectedInChosenUp method in PreviewTabViewModel: The method overwrites somehow the contents of the TextBasedPreviewLayout with the content of the last PreviewLayout in the list, if the last item in the list is a CitationStylePreviewLayout.

Be careful. JabRefPreferences is some sort of logic-model mixture. It's not yet untangled. There should not be a Property in JabRefPreferences, as this class should only provide preferences objects (like PreviewPreferences) and store them. Also, if you save only the name of the style, you also have to provide some mechanism to store the TextBasedPreviewLayout (The "Preview" style)

[calixtus]

But on the other hand, it also happens if you drag-n-drop an item. So maybe it's somewhere in an implementation both methods selectedInChosenUp and dragDroppedInChosenCell share?

Have fun bug-hunting! This is a tricky one to catch, but should be an easy one to fix. 😉

[leitianjian]

Wow, how deep insight you have provided. Many thanks for your instructions. I will try again as soon as possible.

Yeah, I think the JabRef needs more comments on source code. I have taken a long time on understanding what is the function of some method and field, which also easily make more mistakes.

Fortunately, the JabRef main contributors are so kind and active, who have the ability to provide the comments and deep insight for newcomers. Thank you very much.

[leitianjian]

Hi, I have tried to fix the bug. Following is something I have found:

I found the source of the bug is the chosenListProperty.remove() method. I cannot understand why, but it happens.

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

Lines 253 to 258 in bc57d22

	for (int oldIndex : selected) {
	boolean alreadyTaken = newIndices.contains(oldIndex - 1);
	int newIndex = ((oldIndex > 0) && !alreadyTaken) ? oldIndex - 1 : oldIndex;
	chosenListProperty.add(newIndex, chosenListProperty.remove(oldIndex));
	newIndices.add(newIndex);
	}

modify line 256 & 278 to Collections.swap(chosenListProperty, newIndex, oldIndex);
This modification can fix the wrong modification of Preview when we trying to press the up/down arrow.

BUT
The method using remove()/removeAll() function all can lead to the wrong modification of Preview problem.
If the Preview is the first cell and you drag the last cell from chosen to avail list or just drag the last cell up, you can find the bug remains.

I think there are three solutions:

1. Modify the TextBasedPreviewLayout to immutable class
2. Change all remove()/removeAll() to other API (I think it is impossible)
3. Try to figure out why remove() method leads to this bug (I tried with debug mode, but the invoke chain is too deep)

When I test with this bug, some other bugs also happened. (Preview run away when I drag other chosen cells to avail)
I think this class (PreviewTabViewModel.java) should be reviewed more carefully. And both solutions are not easy to fix the bug. Maybe there are some triky things I haven't found ?
Thanks for your instructions in advanced.
@calixtus

[calixtus]

Hi @leitianjian , thanks for debugging this code.
A bit of an obstacle here is, that the user should be able to drag multiple items in the list.
So your solution with Collections.swap would only work for one item.
Also making the TextBasedPreviewLayout immutable does not solve the issue with remove() and removeAll().

What different API did you think of?

[leitianjian]

Yeah, The Collections.swap only can solve the button of up/down.

Immutable TextBasedPreviewLayout I think can solve the issue. From my view, the remove()'s bug coming from unexpected modify. Every cell except Preview is the read-only. And Preview is the unexpected modified cell.

I just list the solution that may work. From my view (I'm not very familiar with JFX) there are not other APIs that can solve this bug.

[calixtus]

Ok, go ahead, give it a shot and maybe push your branch as a draft PR, so we can take a look at it together. 👍

[leitianjian]

Thanks, I will try it. It's my pleasure to hear your advice. If you have any idea about this bug, please tell it to me. :D