Integration Mr. DLib#2189
Conversation
…content not displayed well
…. Issue: tab loads only one time - a solution for that should be implemented as soon as possible.
lenhard
left a comment
There was a problem hiding this comment.
Thanks for your PR! I added comments at a few places in your code. I know this is a prototype and not the final solution to be merged. I commented several details nevertheless to give you an impression of what you need to look out for.
For some weird reason, the travis build did not kick of for your branch. @JabRef/developers We need to look into this. A successful travis build is an absolute requirement. All tests should be successful and the AstrophysicsDataSystemTest should already work if you merge the current master into this branch (please do that). The LocalizationConsistencyTest fails because you used new localizations in your code, but did not add them to the localization files. All other tests also need to work, most importantly the BibtexDatabaseWriterTest. What counts, in the end, is the result of the travis build.
There are some architectural things to improve in your code. Not all code should go into gui, but the code should be nicely separated with a proper separation of concerns.
Unfortunately, I get a 404 when clicking your link above, so I cannot see your sample XML.
I cannot give you a very good hint regarding the update bug, unfortunately. However, things might be fixed when you hook into the threading model used by JabRef instead of a standalone thread.
You generated the distributed code using glassfish, right? As indicated in my comments, parts of that will need to be rewritten using unirest.
Regarding the content of the tab: As you know from the forum, we are not really happy with just transmitting HTML. Ultimately, whatever format you get back from the server, that data should be parsed into a collection of BibEntry. So a nicely parseable format would be of great advantage and JabRef already provides a number of parsers for the most important data formats. You can of course introduce a custom format, but then you would need to write the parser. Anyway, the resulting list of BibEntry could then be nicely layouted in the tab, which may of course also include some HTML, but this would need to be built on the side of JabRef. Please note that we are currently integrating JavaFX, so you will have quite advanced features at hand. When we have such a list of BibEntry, we can also build all kinds of nice integrating features with the rest of JabRef.
Finally, some thoughts from myself regarding aspects from the forum post: I understand that you want to be as flexible as possible and not link too deeply into the JabRef code. At the same time, we do not want to give away control of how the related articles tab looks at runtime. At the very least, we want to be able to sign off visual changes there. A relatively straight forward solution to this might a new library. You could move your parsing and layouting code into that library and keep it completely separate from JabRef. On the JabRef side, you would integrate the code exactly once (by, say, creating a MrDlibPane and a MrDlibFetcher at the right places) and we would include the library into our dependencies. Once uploaded to jcenter, you could update the visualization whenever you feel like and all we would have to do would be to upgrade the version number in our gradle file. In theory, this could keep everything nicely separate. You would not have to know too much about JabRef code, would be flexible, we would have a last say on our layout, and we would not have to fix and correct your visualization code when something breaks. What do you think?
More input from @JabRef/developers is also very welcome!
| compile 'org.mozilla:rhino:1.7.7.1' | ||
|
|
||
| // communication with Mr. DLib | ||
| compile 'org.glassfish.jersey.core:jersey-client:2.23.2' |
There was a problem hiding this comment.
If I understand the PR correctly, you use glassfish for REST calls, right? We already have the following dependency for REST support: 'com.mashape.unirest:unirest-java:1.4.9'
Since there is (hopefully) no need for two different REST libraries, please use the one we already have.
| title = entry.getField("title").toString().replaceAll("\\{|\\[|\\]|\\}|(Optional)", ""); | ||
|
|
||
| //localization is not yet set | ||
| relatedArticlePanel.setName(Localization.lang("Related articles")); |
There was a problem hiding this comment.
If you add a new localization entry, you also have to add the entry to the localization files. Here is an explanation how this works: https://github.com/JabRef/jabref/wiki/Code-Howtos#using-localization-correctly
|
|
||
| //Starting the Thread that makes the request to the server | ||
| Thread t = new Thread(relatedArticleTab); | ||
| t.start(); |
There was a problem hiding this comment.
To get multi-threading, you will need to hook into the parallelization framework we have in place. Have a look at AbstractWorker or maybe FindFullTextAction as an example.
We use a more or less centrally managed threading framework to make sure we can control it properly (choosing an appropriate thread pool). Moveover, spawning off threads at all levels and code is just a premier recipe for dead locking
|
|
||
| //TODO native component, check: how does it work | ||
| tabbed.addTab(Localization.lang("Related articles"), IconTheme.getImage("mdl"), | ||
| relatedArticlePanel, Localization.lang("Show/edit Related articles")); |
There was a problem hiding this comment.
Regarding localization, see above.
| *Works only 5 times. TODO: Check why | ||
| * | ||
| */ | ||
| public class EntryEditorTabRelatedArticles extends JEditorPane implements Runnable { |
There was a problem hiding this comment.
This class does multiple things: GUI-related aspects and fetching of data. This should be separated. Please have a look at our architecture: https://github.com/JabRef/jabref/wiki/High-Level-Documentation
While your gui-related code should of course stay in the gui package. The code related to fetching data should go into an appropriate part of logic.
| public void run() { | ||
| System.out.println("----- Thread Started -----"); | ||
| try { | ||
| SSLContext sc = SSLContext.getInstance("TLSv1"); |
There was a problem hiding this comment.
I guess the following is just a dummy implementation of a certificate validation and should be replaced by a proper one in the end.
| * Sets the default content. Like a "loading screen". | ||
| * With a gif as animation (gif is only a placeholder and can be adjusted at any time). | ||
| */ | ||
| private void setDefaultContent() { |
There was a problem hiding this comment.
There is quite a lot of potential for refactoring in this method, which can be subdivided into a number of small methods.
|
|
||
| } catch (NoSuchAlgorithmException e) { | ||
| System.out.println("NoSuchAlgorithmException"); | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
In the same fashion as there should be no sysouts in the code, e.printStackTrace() is equally an absolute no-go. This should also be turned into logging.
| } | ||
|
|
||
| //Parsing the response with a SAX parser | ||
| try { |
There was a problem hiding this comment.
Content parsing should be extracted into a new class.
| this.htmlContent = htmlContent; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Because I cannot comment on the following file, I'll leave a comment here: gifs should not be located in the source tree. They should go into resources.
|
After thinking about the design some more, I came to the conclusion that the data transmission between Mr.DLib and JabRef should be implemented as a fetcher. After all, what we want to do is to transmit a search string and get back a collection of bibliographic data. That is exactly what we already have implemented in our fetcher framework and a variety of If you reuse this infrastructure, it will be quite easy to integrate Mr.DLib in various other places of JabRef as well, such as the web fetch dialog. Essentially, this should work out of the box. It would be possible to access MrDLib in the same fashion as Google Scholar, IEEExplore, the ACM digital library, etc. A |
|
I strongly agree with @lenhard! The implementation as a fetcher would be ideal. Probably not as a SearchFetcher but as a kind of hybrid with FulltextFetcher: insert a BibEntry (or a bunch of them?) and get a list of BibEntry (the recommendations). This should be the only place in JabRef which knows about MrDLIB. The presentation of the recommended BibEntries can then happen via the Layouter (similar to the entry preview) or by implementing a new user interface (recommended). |
|
Just to provide two-way linking: Discussion on discourse: http://discourse.jabref.org/t/integrating-related-articles-from-mrdlib-into-jabref/240/34 |
|
So just to document what we have decided in the call: The MrDLib team will implement a fetcher for MrDLib and take care about transforming whatever format they provide into @tobiasdiez Could you please provide some guidance as to how to best implement parallelism for the invocation of the fetcher in the gui? |
|
@lenhard you suggested a to implement a
Should I implement this class or stick to the |
|
In general
The semantics are also more clear with a new interface. |
|
Now @tobiasdiez beat me in answering speed :-) But why not just reuse an @stefanfeyer You can also reuse the On an additional note, it seems that the parallelism part for the fetchers is not as sophisticated as I had hoped. The current fetching mechanism is implemented via a |
|
@lenhard oh, I forgot about the |
|
Well, as we are not actually planning to integrate more recommender systems, I would say a second interface that duplicates the functionality is not really needed. I think @stefanfeyer should go for the |
|
Dear JabRef Team, Kind Regards, Stefan Feyer |
| // need to use snapshots as the stable version is from 2013 and doesn't support v1.0.1 CitationStyles | ||
| compile 'org.citationstyles:styles:1.0.1-SNAPSHOT' | ||
| compile 'org.citationstyles:locales:1.0.1-SNAPSHOT' | ||
| compile 'de.undercouch:citeproc-java:0.6' |
There was a problem hiding this comment.
We is this needed as we are already using 1.0.0-SNAPSHOT (see line 125/132)?
There was a problem hiding this comment.
I didnt change sth in this gradle file, i just deleted my dependency i added last time. it should be the original from your master branch. i will investigate why there is a change, this is not with purpose.
| * @return | ||
| */ | ||
| private String formatTitleFromBibEntry(BibEntry selectedEntry) { | ||
| return selectedEntry.getField("title").toString().replaceAll("\\{|\\[|\\]|\\}|(Optional)", ""); |
There was a problem hiding this comment.
|
Hi Stefan, Without going into the details of the code: Generally the fetcher itself is following the correct approach. However, letting the fetcher implement As I have not participated in the calls I'm not sure whether this was a topic. But another aspect which is critical for me is that in my opinion loading the related entries should NOT be performed upon opening an EntryEditor but at the time of switching to the "Related Entries" tab. Two reasons: 1. Efficiency on just cycling through without being interested in the related entries. 2. Privacy of our users. Regards |
|
Just a few additions to the comment by @matthiasgeiger (which is entirely correct): The code relating to the In the calls, we did not really talk about when the fetcher should be invoked. @matthiasgeiger is correct of course: Doing this in the background for all entries would be a significant privacy invasion. It should happen when a user clicks on the related article tab (indicating his or her will to view related articles). Hence, an action that fires when the related articles tab becomes selected that triggers the |
|
If this problem is really dependend only on the JDK/JRE version, it should
not be a problem then 101 OpenJDK should also have it since Update 1ß
2017-01-30 13:12 GMT+01:00 stefanfeyer <notifications@github.com>:
… @tobiasdiez <https://github.com/tobiasdiez> Will our user facing the same
problem?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2189 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AATi5GQjmsR2iC9_sa8DM-ll_QxWpevQks5rXdO8gaJpZM4KebTX>
.
|
|
No SSL error with |
|
Ok, I can confirm that this depends on the Java runtime. I just installed JDK 8u121 and the SSL error is gone. It is fine to require the most recent version of Java, but that means that a lot of users will run into this problem. Maybe we should catch I went through my list a little with the related articles tab and sometimes I get such an exception for an entry. In this case, the related articles tab is just empty: Also, I have to say that, similar as for @koppor, I have the feeling that there is no relation whatsoever between the articles recommended in the related articles tab and my actual entries. Honestly, if I saw these recommendations as a user I would want to turn of the feature as quickly as possible. Sorry for the harsh description, this is not meant to trash MrDLib, but this is what I would think as a user. EDIT: Many of the recommendations for my (completely english) reference list in computer science seem to be german articles from law magazines. That is profoundly weird. |
|
With "Recommendations totally out of scope" I meant that the recommended papers are not fitting the area of the original paper at all. They are computer science, but not in the field of business process management or related areas. |
|
@koppor we are working on that :) |
|
@stefanfeyer On a positive note: The replies from MrDLib come in very quickly :-) |
|
Just implement the fix suggested in #2189 (comment) and it should work also under older java runtimes. And because 101 was released only last summer, I think there are a lot of users which have not yet updated to it. |
|
I implemented the fix. And reselved the errors from the test where possible. Why couldn't I add a help page? Or if that is not suitable, where to put it in the help page? |
|
I don't understand "Why couldn't I add a help page?". Isn't https://github.com/JabRef/help.jabref.org/blob/gh-pages/CONTRIBUTING.md helpful? I would suggest adding a screenshot to https://help.jabref.org/en/EntryEditor and explain the new tab in a new section in that help file. |
lenhard
left a comment
There was a problem hiding this comment.
I have played around some more with the current status and the exceptions mentioned above are gone. The tab presents articles for all entries that I tested, so overall it is functional. The recommendations are unfortunately still unrelated to my entries, but I hope this will change.
Have a look at the travis-ci build: You still have checkstyle errors (wrong sort order of import statements). This needs to be fixed. Also, please add the german localization for your PR and for any other languages that you are speaking.
|
|
||
| public void fixSSLVerification() { | ||
|
|
||
| LOGGER.warn("Fix SSL exception by accepting ALL certificates"); |
There was a problem hiding this comment.
This prints a message into the log file every time the method is called. Please reduce the log level to debug.
|
made the adjustments. our deploy to the productions server will happen this week. then we change the url where we requesting the recommendations. Then we can go live. |
| All_subgroups_(recursively)=All_subgroups_(recursively) | ||
|
|
||
| What_is_Mr._Dlib?=What_is_Mr._Dlib? | ||
| What_is_Mr._Dlib?=What_is_Mr._DLib? |
There was a problem hiding this comment.
The english translation must be EXACTLY the same as the key.
Thus, if you wan't to change the casing to DLib you'll have to change the key (in all files) as well.
|
|
We are currently sitting together at JabRef and decided to merge this now :-) We will change to the new URL after the merge. Thanks for your contribution! |
|
Thanks for the useful integration! Just a small improvement request. Is it possible to change the font used in the Related article window? The current one is really ugly. There are variations of the line widths (that appears less pronounced in the attached screenshot) and there is no antialiasing. One example is the S. Compare also the P and e of the first "Pedagogy" word. |
|
Hi @crystalfp Thanks for your feedback, but actually JabRef is the wrong place to report this. The content of the related article tab is just HTML that we load from MrDLib. They determine how it looks like. I think it's best if you report this here: https://github.com/Mr-DLib/mdl-server But maybe @Joeran would like to comment on this? |
|
Done, thanks! |
|
thx, i commented in BeelGroup/Mr.-DLib-Server#81 |
Integration of the recommendersystem Mr. DLib
Reason for pull request
We from Mr. DLib would like to integrate a recommendation system into JabRef. We believe that both sides will benefit from this.
This pull request is the basis for further and deeper discussion of what is the right way to implement this feature.
Changes made to JabRef
A new tab in the EntryEditor is added on the very right with a small MDL icon. A loading screen is shown in this tab while a Thread makes a request to the RESTful webservice of Mr. DLib. With the received response, a SAX parser parses the XML the response contains. Currently the html-code inside this XML is taken to display the six recommendations received from the server.
If you want to have a look how the XML delivered by the server please click the following link: https://api-dev.mr-dlib.org/v1/documents/gesis-smarth-0000003299/related_documents/
Notes
Please note, that this is only an example recommendations, the server-structure for handling the request is in implementation. Once this done, the title of the selected Item represents the request and based on this MR DLib will respond with matching articles.
Please note, that the appearance is only a suggestion. We are open and pleased for further input.
Please note, that code style is up to change. If you have improvements, they are all welcome.
A bug i couldn't deal with is, that the new "Related Articles" tab I created is only called up to 5 times. After that it don't get updated. It would be very helpful if someone could give me a hint where I have a mistake in my mental model of JabRef.
Current screenshot
gradle localizationUpdate?I'm aware that gradlew check failed at:
BibtexDatabaseWriterTest. roundtrip
BibtexDatabaseWriterTest. roundtripWithUserComment
BibtexDatabaseWriterTest. roundtripWithUserCommentAndEntryChange
BibtexDatabaseWriterTest. roundtripWithUserCommentBeforeStringAndChange
AstrophysicsDataSystemTest. testPerformSearchByFamaeyMcGaughEntry
zbMATHTest. searchByQueryFindsEntry
LocalizationConsistencyTest. findMissingLocalizationKeys
XMPUtilTest. testCommandLineSeveral
I will check if this is caused by my work.
Kind regards, Stefan Feyer