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

Integrate mrdlib redesign #4361

Merged
merged 25 commits into from
Oct 26, 2018
Merged

Integrate mrdlib redesign #4361

merged 25 commits into from
Oct 26, 2018

Conversation

conorfos
Copy link
Contributor

@conorfos conorfos commented Sep 27, 2018

Integrated the new Mr. DLib into JabRef:

  • Mr. DLib now provides recommendations in JSON
  • JabRef presents these using JavaFX
  • In line with GDPR, users accept that they consent for data to be sent to Mr. DLib
  • Included a new setting in Entry Editor Preferences to allow users to retract their consent for the above
  • Removed setting of html_representation field for recommendations - UI is now populated from the data fields themselves.
  • Changed the default color of JavaFX Hyperlinks to '-jr-theme'

Note:
The Mr. DLib Dev environment - used for JabRef Dev versions - is quite a bit slower than the production environment.


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

MrDLibImporter and MrDLibFetcher updated to support JSON and new Mr.
DLib format. Tests updated and passing.
Created dialog to ask for consent to sed data to Mr. DLib
Created preference setting to keep track of consent
Updated the recommendations GUI to use JavaFX
Changed privacy dialog text and allowed the text to wrap.
Replaced if/else with in-line conditional
Recommendation html_representation field no longer set as it is not used
@Siedlerchr
Copy link
Member

Thanks for your contribution. Could you please add some screenshots how it now looks?

testMax = testMax.replaceAll("&", "");
inputMin = new BufferedReader(new StringReader(testMin));
inputMax = new BufferedReader(new StringReader(testMax));
testInput = "{ \"label\": { \"label-language\": \"en\", \"label-text\": \"Related Items\" }, \"recommendation-set-id\": \"1\", \"recommendations\": { \"74021358\": { \"abstract\": \"The theme of diploma is the preservation of the rural lands with the spatial development strategy on the case of the Hrastnik commune. The rural land with the potential production which is appropriate for the rural use is very important natural source. The natural source is very important for the quality of living and the spatial development. For this reason the assurance of the validity of the usage of the rural lands and to preserve the rural land is one of the basic ways of the strategy of the spatial development of Slovenia. The task is compose into two parts. The first part is theoretical. It informed us with the situations and achievements of the spatial development in Slovenia. In brief it shows the professional starting-point, which are for the use of the legally regime to preserve the rural lands and have been made by Dr. A. Stritar and they are still actual. In the review of the new and old rural land legislation it shows the adaptation of the rural lands through the periods until today. The second part treats the practical case of the preservation of the rural lands on the case of the Hrastnik commune. Based on the professional foundation for the classification of the rural lands presents the spatial analyses with reference to the rural lands and the category of the rural lands of the commune. At the end of the chapter it shows the part of Brnica. Here the commune of Hrastnik in the procedure of the preparation of new spatial-act wants to change the purpose of the best rural lands with the high production potential for the enlargement of the settlement and adaptation of the cemetery\", \"authors\": [ \"Sajovic, Marija\" ], \"date_published\": \"21-06-2006\", \"item_id_original\": \"12088644\", \"keywords\": [ \"visoko\\u0161olski program Geodezija - smer Prostorska informatika\" ], \"language_provided\": \"sl\", \"recommendation_id\": \"1\", \"title\": \"The protection of rural lands with the spatial development strategy on the case of Hrastnik commune\", \"url\": \"http://drugg.fgg.uni-lj.si/701/1/GEV_0199_Sajovic.pdf\" }, \"82005804\": { \"abstract\": \"The theme of my diploma thesis is the engagement of the volunteers in the solution to the accidents in the South-Moravia region. In my diploma thesis I am involved in the issue of the engagement of the volunteers in the solution to the accidents in the South-Moravia region. I assume that the utilization of the potential of the volunteers is one of the possibilities of the acquisition of the forces, which could be used for the recovery of the consequences, post-traumatic care to struck persons and other activities within the solution to the accidents, all parallelly with the actions of the professional teams of the integrated rescue system bodies. The needs of the population in case of the accidents are rather differentiated, and the engagement of the volunteers completes the appointed forces and means of the integrated rescue system bodies. At present time, the adequate legal framework, modifying the engagement of the volunteers in the solution to the accidents, does not exist in the Czech Republic. Non-governmental non-profit organizations are united in the South-Moravia region into the Panel of the South-Moravia Region, which establishes the platform and enables the South-Moravia Region more effective engagement of the non-governmental non-profit organizations. The volunteers, organized in the Panel of the South-Moravia Region, are engaged in the solution to the accidents and the trainings organized in the level of the region, municipalities with widened competence and the integrated rescue system bodies\", \"date_published\": null, \"item_id_original\": \"30145702\", \"language_provided\": null, \"recommendation_id\": \"2\", \"title\": \"Engagement of the volunteers in the solution to the accidents in the South-Moravia region\" }, \"82149599\": { \"abstract\": \"English Annotation (The work abstract) 5 key words: The Father, The Son, The Holy Spirit, The Holy Trinity, Saint John of the Cross \\\"The Only Father's Word\\\" The Relationship of the Father and the Son in documents of Saint John of the Cross The author presented the life of the saint in the first part of the work with the intention to put some key events of his life to the connection with the testimony of his texts. In the next part she analyzed those texts in the documents which are devoted to the relationship of the Father and the Son. Here we mainly talk about the twenty-second chapter of the second book, the Ascent of Mount Carmel, in which \\\" The Son is the only Father's word\\\". The father is active only in one way - begetting the Son; the begetting itself is Love, which is The Holy Spirit. The Son loves the Father and through this love he makes the Father love, he isn't passive. In the Romances, Saint John shows how the relationship of the Father and the Son in the Spirit is realized in the creation and incarnation. The poem the Source deepens our understanding of Son and Holy Spirit's proceeding from the Father and his acting in the Eucharist. In the third and the last part of the work, she made the synthesis of the knowledge gotten by the analysis of the given texts and she came to a conclusion that the..\", \"date_published\": null, \"item_id_original\": \"97690763\", \"language_provided\": null, \"recommendation_id\": \"3\", \"title\": \"\\\"The only Father's word\\\". The relationship of the Father and the Son in the documents of saint John of the Cross\", \"url\": \"http://www.nusl.cz/ntk/nusl-285711\" }, \"84863921\": { \"abstract\": \"This thesis provides an analytical presentation of the situation of the Greek Church of Cyprus, the Morea and Constantinople during the earlier part of the Frankish Era (1196-1303). It examines the establishment of the Latin Church in Constantinople, Cyprus and Achaea and it attempts to answer questions relating to the reactions of the Greek Church to the Latin conquests. It considers the similarities and differences in the establishment in Constantinople, the Morea and Cyprus, the diocesan structure, agreements regarding the fate of the Greek ecclesiastical properties, the payment of tithes and the agreements of 1220-1222. Moreover it analyses the relations of the Greek Church of Cyprus, the Greek Church of Constantinople and the Morea with the Latin Church. For instance it details the popes' involvement in the affairs of the Church in these three significant areas, the ecclesiastical differences between the Greek and the Latin Church, the behaviour of the Greek patriarchs, archbishops and bishops within the Greek Church, the reaction of the Greeks towards the establishment of the Latin Church, and significant events such as the martyrdom of the thirteen monks of Kantara and the promulgation of the Bulla Cypria. The third topic area pertains to the relationship of the Greek Church of the Morea, Constantinople and Cyprus with the secular authority. It discusses the attitude of the king of Cyprus, the rulers of the Morea and the emperor of Constantinople towards the problems between the Latin and Greeks, the relationship of the Latin nobility with the Greeks, and the involvement of the crown regarding the ecclesiastical property and possible explanations for the attitude of the Latin crown towards the Greeks\", \"authors\": [ \"Kaffa, Elena\" ], \"date_published\": null, \"item_id_original\": \"19397104\", \"keywords\": [ \"BX\", \"D111\" ], \"language_provided\": \"en\", \"recommendation_id\": \"4\", \"title\": \"Greek Church of Cyprus, the Morea and Constantinople during the Frankish Era (1196-1303)\" }, \"88950992\": { \"abstract\": \"1. The phylogeny map is the 4th-dimensional extension of a protoplast in which the generations repeated continuously. One generation in the phylogeny is the three-dimensional extension of the protoplast in certain period which is limited by a resting stage accompanying the reproductive phase. 2. It is considered that the ancestor of the seed plants was a hygrophytic multinuclear thallus having the structure like the cotyledonary ring including the hypocotyl in the plants of the present age. 3. In the second stage of the development in the phylogeny the thallus became uninuclear multicellular one, and the water storage cells differentiated in the body which gave the aerophytic habit for the plant. 4. Aerophytic habit gave the great change to the plant not only morphologically but also in the physiological natures. The epidermis having stomata differentiated. The mesophyll became to serve for the photosynthesis instead of the epidermis. The arrest of the escape of the metabolites from the surface of the body, the accumulation of the photosynthetic products, and the evaporation of water from the body surface lead the\\ndifferentiation of conductive tissues, the phloem and xylem. 5. The transfer of the nutrients and also the metabolites, especially such as the activator of the nuclear division, influenced to differentiate the secondary meristem. The growing point of the root and the primordium of the first node of the stem including the leaves and internode produced from the meristems. The embryo and bud formation are considered. 6. The appearance of the bud lead the formation of the complicated reproductive organ called the flower. The qualitative differences between the MiSI, and MaSI, and the migration of the SI were considered. 7. The ancestor of the Pteridophyte must be entirely different from that of the seed plants. It was a uninuclear multicellular plant having the growing point at the distal end of the body\", \"authors\": [ \"Yasui, Kono\" ], \"date_published\": null, \"item_id_original\": \"38763657\", \"language_provided\": null, \"recommendation_id\": \"5\", \"title\": \"A Phylogenetic Consideration on the Vascular Plants, Cotyledonary Node Including Hypocotyl Being Taken as the Ancestral Form : A Preliminary Note\" } }}";
Copy link
Member

Choose a reason for hiding this comment

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

Please no abstracts in the tests, as abstracts are copyrighted by the journals

@@ -86,6 +165,11 @@ public boolean shouldShow(BibEntry entry) {

@Override
protected void bindToEntry(BibEntry entry) {
setContent(getPane(entry));
// Ask for consent to send data to Mr. DLib on first time to tab
if (JabRefPreferences.getInstance().getBoolean(JabRefPreferences.ACCEPT_RECOMMENDATIONS)) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use the EntryEditorPreferences preferences object for the check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Christoph, thanks for your help.

I had been using EntryEditorPreferences, but switched because the object doesn't get updated until the application is restarted. This resulted in the dialog still appearing after accepting, until the application was restarted.

Is there a way of doing this such that the preferences are refreshed on each load of the tab?

Copy link
Member

Choose a reason for hiding this comment

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

At the moment, this is not possible. However, please use the EntryEditorPreferences, we are trying to get rid of the global JabRef preference class. It is a bit of a nuance that you need to restart JabRef, but in this case I prefer the better code quality.

@conorfos
Copy link
Contributor Author

Screenshots of the recommendations, preferences dialog, and privacy dialog:
1
2
3

Removed html representation test
Removed abstracts from tests
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

The code looks good to me and I've only a few minor suggestions for improvement. Please also merge with the latest changes from master.

StackPane root = new StackPane();
root.setStyle("-fx-padding: 20 20 20 20");
Copy link
Member

Choose a reason for hiding this comment

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

Please set this in the css file.

JabRefDesktop.openBrowser(entry.getField(FieldName.URL).get());
} catch (IOException e) {
LOGGER.error("Error opening the browser to: " + entry.getField(FieldName.URL).get(), e);
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

Please use one of the dialogService.showError... methods to show the error to the user instead of the printStackTrace.

vbox.getStyleClass().add("gdpr-dialog");
vbox.setSpacing(20.0);

Button button = new Button("I Agree");
Copy link
Member

Choose a reason for hiding this comment

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

Use Localization.lang for text that needs to be translated (here and everywhere).

vbox.setSpacing(20.0);

Button button = new Button("I Agree");
button.getStyleClass().add("accept-button");
Copy link
Member

Choose a reason for hiding this comment

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

button.setDefault(true) should achieve the same.

@@ -86,6 +165,11 @@ public boolean shouldShow(BibEntry entry) {

@Override
protected void bindToEntry(BibEntry entry) {
setContent(getPane(entry));
// Ask for consent to send data to Mr. DLib on first time to tab
if (JabRefPreferences.getInstance().getBoolean(JabRefPreferences.ACCEPT_RECOMMENDATIONS)) {
Copy link
Member

Choose a reason for hiding this comment

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

At the moment, this is not possible. However, please use the EntryEditorPreferences, we are trying to get rid of the global JabRef preference class. It is a bit of a nuance that you need to restart JabRef, but in this case I prefer the better code quality.

return uri.toString();
} catch (URISyntaxException e) {
LOGGER.error(e.getMessage(), e);
}
return "";
}

private String getMdlUrl() {
Version currentVersion = Globals.BUILD_INFO.getVersion();
Copy link
Member

Choose a reason for hiding this comment

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

please pass Globals.BUILD_INFO as a constructor argument (we are trying to remove the Globals class). If I'm not mistaken, then the Fetcher already accepts a version string as a constructor argument; this should be aligned.


// Get recommendations from response and populate bib entries
JSONObject recommendationsJson = new JSONObject(recommendations);
recommendationsJson = recommendationsJson.getJSONObject("recommendations");
Copy link
Member

Choose a reason for hiding this comment

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

JSONObject recommendationsJson = new JSONObject(recommendations).getJSONObject("recommendations"); is somewhat easier to understand.

@conorfos
Copy link
Contributor Author

Requested changes made, and merge with latest from Master complete.

@conorfos
Copy link
Contributor Author

@tobiasdiez Hi Tobi, just for the purposes of planning on our side:
Is there anything further required from us for this pull request?
When do you think this will be merged and released?
Thanks!

@Siedlerchr
Copy link
Member

Please fix the checkstyle issues. From my point it looks good now. (Just click on the travis erros)
And you seem to have a java error in your test:
/home/travis/build/JabRef/jabref/src/test/java/org/jabref/logic/importer/fetcher/MrDLibFetcherTest.java:23: error: incompatible types: String cannot be converted to Version

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Sorry, for the late response. The code looks good to me. Please fix the build issues on travis and then we can merge.

setContent(getRelatedArticlesPane(entry));
}
button.setOnAction(event -> {
JabRefPreferences prefs = JabRefPreferences.getInstance();
Copy link
Member

Choose a reason for hiding this comment

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

Please use the EntryEditorPreferences which are already passed via constructor. Apart form that lgtm

Copy link
Collaborator

Choose a reason for hiding this comment

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

But shouldn't this setting not go into the JabRefPreferences and not the preferences of the EntryEditor? When I see this correctly in the above screenshot, this setting was supposed to be available main preferences.

@halirutan
Copy link
Collaborator

I forgot how the exact procedure is to handle new localization strings. Wasn't there a script that can do this automatically? Anyway, it seems that the messages used in org.jabref.gui.entryeditor.RelatedArticlesTab need to be added to the localization files.

@Siedlerchr
Copy link
Member

You just need to add the new localization strings to the english language file. The rest is done via crowdin

@tobiasdiez tobiasdiez merged commit f1c29c2 into master Oct 26, 2018
@tobiasdiez tobiasdiez deleted the integrate-mrdlib-redesign branch October 26, 2018 07:53
@conorfos
Copy link
Contributor Author

Thank you very much everyone for fixing up those last few bits. Looking forward to the next release!

Siedlerchr added a commit that referenced this pull request Oct 27, 2018
* upstream/master: (30 commits)
  Update README.md (#4419)
  Fix generate bibtex key overwrite warning dialog (#4418)
  Fix group hit counter when adding entries (#4413)
  Update README.md
  fix java.nio.file.FileSystemNotFoundException and reorganize code (#4416)
  fixed an issue where corresponding groups are sometimes not highlighted when clicking on entries (#4404)
  Add a few journal abbreviations (#4412)
  Fix display of radio buttons (#4411)
  Update issue templates
  Delete ISSUE_TEMPLATE.md
  Update issue templates
  Integrate mrdlib redesign (#4361)
  fix "save" button in preference dialog not response (#4406)
  show dialog before creating entry (#4405)
  Scrollbar invisible in Preferences -> BibTex Key Pattern (#4287) (#4398)
  Updates (#4402)
  Fixes for all mods exporter tests (#4396)
  Fix EntryType dialog not closed after generate button (#4390)
  Make rank image narrower (#4395)
  Fix showing multiple icons for one menu entry - fixes #4384 (#4392)
  ...
Siedlerchr added a commit that referenced this pull request Oct 29, 2018
* upstream/master: (54 commits)
  Update README.md (#4419)
  Fix generate bibtex key overwrite warning dialog (#4418)
  Fix group hit counter when adding entries (#4413)
  Update README.md
  fix java.nio.file.FileSystemNotFoundException and reorganize code (#4416)
  fixed an issue where corresponding groups are sometimes not highlighted when clicking on entries (#4404)
  Add a few journal abbreviations (#4412)
  Fix display of radio buttons (#4411)
  Update issue templates
  Delete ISSUE_TEMPLATE.md
  Update issue templates
  Integrate mrdlib redesign (#4361)
  fix "save" button in preference dialog not response (#4406)
  show dialog before creating entry (#4405)
  Scrollbar invisible in Preferences -> BibTex Key Pattern (#4287) (#4398)
  Updates (#4402)
  Fixes for all mods exporter tests (#4396)
  Fix EntryType dialog not closed after generate button (#4390)
  Make rank image narrower (#4395)
  Fix showing multiple icons for one menu entry - fixes #4384 (#4392)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants