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

Added change listener for crossref field #1645

Merged
merged 2 commits into from Aug 12, 2016

Conversation

Projects
None yet
3 participants
@oscargus
Contributor

oscargus commented Jul 29, 2016

Another WIP just to show what is coming (as I may be unavailable for a week or so now).

Change listener that updates the crossref field when the key changes. In the longer run this will be extended to all field with keys, see #1637.

Will continue with tests and CHANGELOG entries eventually.

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef

@oscargus oscargus changed the title from [WIP] Added change listener for crossref field to Added change listener for crossref field Jul 30, 2016

@oscargus

This comment has been minimized.

Contributor

oscargus commented Jul 30, 2016

Potential issue: when deleting an entry all references to it in crossrefand related is removed, which is good. However, when undo:ing, that information is not restored. Similarly, undoing after removing a BibTeX key doesn't restore crossrefand related. Undoing works for plainly changing the key, as this will trigger new events.

Not sure what the best solution is. Either not removing the keys in the fields, leading to an inconsistent database, or keeping it as it is, that the information is lost. I cannot really see how to actually undo it. It is of course possible to add undo information from the listener, but then it will be a separate undo compound compared to the remove, so one will need to undo twice...

Apart from that, this PR should be ready for review.

InputStreamReader fr = new InputStreamReader(stream, StandardCharsets.UTF_8)) {
ParserResult result = BibtexParser.parse(fr);
db = result.getDatabase();

This comment has been minimized.

@tobiasdiez

tobiasdiez Jul 30, 2016

Member

Please create the database by hand instead of reading it from the file. It also has the advantage that you can save the reference to the entries in private fields.

This comment has been minimized.

@oscargus

oscargus Jul 30, 2016

Contributor

OK! I'll do that.

@oscargus

This comment has been minimized.

Contributor

oscargus commented Aug 2, 2016

Anything else here?

for (String field : keyFields) {
entry.getFieldOptional(field).ifPresent(fieldContent -> {
if (InternalBibtexFields.getFieldExtras(field).contains(FieldProperties.SINGLE_ENTRY_LINK)) {
if (fieldContent.equals(oldKey)) {

This comment has been minimized.

@tobiasdiez

tobiasdiez Aug 2, 2016

Member

Maybe move the contents of the if and else bodies to seperate methods to increase readability.

@Before
public void setUp() throws IOException {
Globals.prefs = JabRefPreferences.getInstance();

This comment has been minimized.

@tobiasdiez

tobiasdiez Aug 2, 2016

Member

prefs not needed here, right?

This comment has been minimized.

@oscargus

oscargus via email Aug 2, 2016

Contributor
@tobiasdiez

This comment has been minimized.

Member

tobiasdiez commented Aug 2, 2016

Looks good to me.

@oscargus

This comment has been minimized.

Contributor

oscargus commented Aug 2, 2016

@oscargus

This comment has been minimized.

Contributor

oscargus commented Aug 9, 2016

The tests will pass again once #1697 is merged.

return new JournalAutoCompleter(fieldName, preferences, abbreviationLoader);
} else {
return new DefaultAutoCompleter(fieldName, preferences);
}
}
public AutoCompleter<String> getPersonAutoCompleter() {
return new NameFieldAutoCompleter(Arrays.asList(FieldName.AUTHOR, FieldName.EDITOR), true, preferences);
return new NameFieldAutoCompleter(InternalBibtexFields.getAllPublicFieldNames().stream().filter(

This comment has been minimized.

@tobiasdiez

tobiasdiez Aug 12, 2016

Member

I propose to move some of the logic to InternalBibtexFields so that you can write InternalBibtexFields.getPersonFieldNames().

This comment has been minimized.

@oscargus

oscargus Aug 12, 2016

Contributor

I was also considering that. Maybe even introduce a constant list as there are for some other properties. Advantage: no need for two long lines of code. Disadvantages: not user configurable (if we ever want that, easy to change though) and, if we add a method, there may be quite a few special methods. Which on the other hand may be better than quite a few two line statements...

I might go for the constant list as there are probably more fields that should count as journal names and then we might include them in one step as well.

This comment has been minimized.

@oscargus

oscargus Aug 12, 2016

Contributor

I introduced it in #1720, so easiest thing is to merge that first and then rebase this one.

@oscargus oscargus merged commit ad6b5cc into JabRef:master Aug 12, 2016

2 checks passed

codecov/project 28.53% (+0.07%) compared to 5267350
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@oscargus oscargus deleted the oscargus:keychangedevent branch Aug 12, 2016

ayanai1 added a commit to ayanai1/jabref that referenced this pull request Sep 5, 2016

Added change listener for crossref field (JabRef#1645)
* Added change listener for crossref field

* Improved AutoCompleterFactory
@Dellu

This comment has been minimized.

Dellu commented Dec 20, 2016

This is dangerious.
I removed a large number of book entries to only update with a new version of them exported from Bookends. All the incollection entries crossrefed with them have lost their crossref field.
I thought the update would not delete the fields: only update when there is change on the key. That is what BibDesk does. It never delets the keys.

Unless you can implement it like the BibDesk, I think it is better to put this feature into the Cleanup tools. The user can call for it only when she wants to update changes. It is dangerous otherwise. I was shocked when I see all of them have lost their link. if not for Timemachine, I would be crying by now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment