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

Fix #628: implement hierarchical keywords #2703

Merged
merged 6 commits into from
Apr 6, 2017
Merged

Conversation

tobiasdiez
Copy link
Member

@tobiasdiez tobiasdiez commented Apr 2, 2017

The automatic keywords group now supports hierarchical keywords.
The changes mainly concentrate on the keywords class...so this may have some unwanted side effects in other parts (but I didn't noticed anything unusual in my tests).

Note: Bib files containing an automatic keyword from prior 4.0 tests can no longer be opened and the lines containing AutomaticKeywordGroup have to be deleted.

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 2, 2017
Copy link
Member

@lenhard lenhard left a comment

Choose a reason for hiding this comment

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

Not really using keywords I cannot meaningfully test this and someone else should jump in for that. The code looks good mostly except for some architecture thing: Do we really want the model to be depending on JavaFX classes? I vaguely remember discussing this and maybe I was overruled by the majority. If so, please point me to where this is documented. If not, please change your code :-)

@@ -2,7 +2,10 @@

import java.util.Set;

import javafx.collections.ObservableList;
Copy link
Member

Choose a reason for hiding this comment

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

I know this is convenient, but I am strongly against it. We do not want the model to be dependent on a GUI technology. If you want this conversion to be done based on ObservableList, it will have to be done elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that we don't want to have GUI technology in our model and logic classes. However, these observable lists and properties have nothing to do with the GUI-related part of JavaFX - you can use them in a command line application if you want. An observable list is just a list with some built-in event listeners, i.e. a different form of List combined with the google event stuff. Similar the properties in in javafx.beans are just fields with event handlers.

Thus I strongly vote for allowing javafx.collections and beans in model and logic.

Copy link
Member

Choose a reason for hiding this comment

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

Decision in devcall: make exception for javafx.collections.*

import java.util.stream.Collector;

import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
Copy link
Member

Choose a reason for hiding this comment

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

Again, no javafx in model

@matthiasgeiger
Copy link
Member

Note: Bib files containing an automatic keyword from prior 4.0 tests can no longer be opened and the lines containing AutomaticKeywordGroup have to be deleted.

Isn't there a possibility to migrate this? From a user point of view this is not really acceptable...

@tobiasdiez
Copy link
Member Author

@matthiasgeiger I formulated it a bit awkwardly. Automatic keywords groups are a new feature in 4.0, so this only hits users of the current development version - not users of 3.8.2.

@tobiasdiez tobiasdiez merged commit 67e457c into master Apr 6, 2017
@tobiasdiez tobiasdiez deleted the groupImprovements branch April 6, 2017 13:07
Siedlerchr added a commit that referenced this pull request Apr 6, 2017
* upstream/master:
  Implement #1359: collect telemetry (#2283)
  Write groups under tag `group` instead of `groupstree` to enhance compatibility with previous versions (#2704)
  Avoid conversion of single underscores (#2711)
  Fix #628: implement hierarchical keywords (#2703)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants