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

Fixes Texgroup's "Library has been modified by another program" #6584

Closed

Conversation

systemoperator
Copy link
Contributor

@systemoperator systemoperator commented Jun 4, 2020

Fixes #6420.
Partially fixes #6585.

} else if (entry.getKey().startsWith(MetaData.FILE_DIRECTORY + "Latex-")) {

must be called before calling

so that the latex file directory is set properly.

  • 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.

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation and the fix! I have a suggestion how to make the code more efficient

@@ -104,6 +100,19 @@ public MetaData parse(MetaData metaData, Map<String, String> data, Character key
metaData.putUnknownMetaDataItem(entry.getKey(), value);
}
}

// process GROUPSTREE and GROUPSTREE_LEGACY at the very end (otherwise it may happen that not all dependent data is set)
for (Map.Entry<String, String> entry : data.entrySet()) {
Copy link
Member

Choose a reason for hiding this comment

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

What about converting the whole switch/case to an if else with the group processing at the end and add it to the other?
This way you wont' need to reiterate over the whole map and you can ensure that it's in order.

(A switch case statement does not guarantee any order, e.g. default can be at the top, and it would only be executed if no cases match).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, my correction is not very beautiful. I shortly thought about other solutions as well. One idea was e.g. to convert data.entrySet() to a List and then searching for those entries and shifting them to the end. What do you think about that?

I'm not sure, if I understood you correctly. Currently, a set gets iterated, where GROUPSTREE/GROUPSTREE_LEGACY could occur before or after setting the Latex directory. While iterating, the entries would also get processed in the same sequence, even when refactoring it to a whole if-else block, or am I missing something? Nevertheless, I will refactor it to a combined if-else now, because it looks much better.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I think I misunderstood it a bit., Thanks for your explanation. It's more clear now.
What about using something like: data.entrySet().stream(). filter(entry,getKey() == Groups... .. || entry.getKey() == Groups...

@systemoperator
Copy link
Contributor Author

@Siedlerchr Before proceeding, I will wait for your recommendation.

@systemoperator
Copy link
Contributor Author

@Siedlerchr How about this?

@calixtus
Copy link
Member

calixtus commented Jun 5, 2020

Can we close this PR and merge the other one #6586 ? I think #6586 includes the commits of this one.
Would be great if you copy 'fixes' from description to the newer one...

@systemoperator
Copy link
Contributor Author

Can we close this PR and merge the other one #6586 ? I think #6586 includes the commits of this one.
Would be great if you copy 'fixes' from description to the newer one...

Yes. Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants