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

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

Closed
Siedlerchr opened this issue Mar 28, 2020 · 12 comments · Fixed by #6372
Closed
Labels
bug Confirmed bugs or reports that are very likely to be bugs preferences

Comments

@Siedlerchr
Copy link
Member

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/:?]
@Siedlerchr Siedlerchr added preferences bug Confirmed bugs or reports that are very likely to be bugs labels Mar 28, 2020
@leitianjian
Copy link
Contributor

leitianjian commented Apr 16, 2020

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
Copy link
Member Author

Siedlerchr commented Apr 17, 2020

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

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

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

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
Copy link
Contributor

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
Copy link
Contributor

leitianjian commented Apr 20, 2020

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.

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

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
Copy link
Contributor

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

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
Copy link
Contributor

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

@leitianjian
Copy link
Contributor

leitianjian commented Apr 28, 2020

Hi, I think I have fixed this issue successfully, waiting for your review :D

koppor pushed a commit that referenced this issue Sep 15, 2022
201e022 Update trends-journals.csl (#6224)
46e6eed Update nottingham-trent-university-library-harvard.csl (#6220)
684bb48 Update politix.csl (#6199)
c484b0b Update mcgill-fr.csl (#6198)
cbcf2f2 Update mary-ann-liebert-vancouver.csl (#6218)
47174f0 Create journal-of-dairy-research.csl (#6195)
fdd1eac Update harvard-anglia-ruskin-university.csl (#6196)
9e384d6 Create estonian-journal-of-earth-sciences.csl (#6194)
afba9b7 Delete moore-theological-college.csl as per university (#6197)
644549f Create acta-medica-philippina.csl (#6192)
6566114 Update rassegna-degli-archivi-di-stato.csl (#6186)
3509a2f Update universidade-federal-de-sergipe-departamento-de-engenharia-de-… (#6187)
de4845f Update mary-ann-liebert-vancouver.csl (#6213)
16828b6 Update ucl-university-college-apa.csl (#6172)
c08613b Update ucl-university-college-harvard.csl (#6173)
028bad4 Create sociologia-ruralis.csl (#6170)
77d428c Update journal-of-plankton-research.csl (#6169)
92e1022 Update and rename dependent/journal-of-the-national-cancer-institute.… (#6168)
120efb1 Update journal of hearing science
e503477 Create new-harts-rules-the-oxford-style-guide-author-date.csl (#6163)
49ab318 Create the-depositional-record.csl (#6159)
f4f6920 Update and rename laser-and-photonics-reviews.csl to dependent/laser-… (#6165)
d8ca4bc Update united-states-international-trade-commission.csl (#6162)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 201e022
@koppor koppor moved this to Done in Prioritization Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bugs or reports that are very likely to be bugs preferences
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants