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

[WIP] Rhar768/feature/23 rest interface #59

Closed
wants to merge 28 commits into from

Conversation

rowanh22
Copy link
Collaborator

@rowanh22 rowanh22 commented Sep 11, 2020

Refs #23

@rowanh22 rowanh22 changed the title Rhar768/feature/23 rest interface [WIP] Rhar768/feature/23 rest interface Sep 11, 2020
@koppor koppor marked this pull request as draft September 11, 2020 04:41
@koppor koppor mentioned this pull request Sep 11, 2020
Copy link
Collaborator

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Minor comments.

Please ensure that CheckStyle (and IntelliJ) is correctly setup at your side. Then, you can press Ctrl+Alt+L to fix the formatting (and Ctrl+Alt+O "Optimize Imports). One "annoying" thing of JabRef's checkstyle config is that there is a space needed after comment start (OK: // comment, not OK: //comment)

If possible, add Tests for the serialization. One can press Ctrl+Shift+T to quickly create test case skelletons.

See org.jabref.logic.bibtex.BibEntryWriterTest for an example test for BibEntry writing.

import java.util.ArrayList;
import java.util.List;

@Path("/root")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to omit /root?

Suggested change
@Path("/root")
@Path("/")

public class RootResource {

@GET
@Path("entries")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please foresee the possibilitiy to work on multiple libraries (libraries/current)

Suggested change
@Path("entries")
@Path("libraries/current/entries")

@Path("entries")
@Produces(MediaType.APPLICATION_JSON)
public String getEntries() {
List<BibEntry> entries = MindMapManager.instance().getBibEntries();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is a bit strange. I would go to the current opened library directly instead of routing the request throught he MindMapManager. (Since one does not need to be that specific here, the more general "object" is enough)

public JsonElement serialize(BibEntry entry, Type typeOfSrc, JsonSerializationContext context) {
JsonObject obj = new JsonObject();

obj.addProperty("type", entry.getType().getName());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please introduce a constant (public static final String JSON_TYPE)

General thing: We need to add two meta information to the JSON. We need to the bibtex content and the meta data type and key. They should not be mixed up. We need to consider that a user can add custom fields.

To be prepared for moree meta data, I would suggest a nested object

{
  "bibtex_metadata": {
    "type": "article",
    "key": "the-citation-key"
  }
}

In case a user adds bibtex_metadata as custom field in the entry editor, then the thing breaks.

Alternative: Do not add special handling for citationkey, because JabRef itself uses that field name.

Another thing:

Shouldn't one use typedEntry.getTypeForDisplay() for type writing (Source: org.jabref.logic.bibtex.BibEntryWriter#writeRequiredFieldsFirstRemainingFieldsSecond)

  TypedBibEntry typedEntry = new TypedBibEntry(entry, bibDatabaseMode);
  typedEntry.getTypeForDisplay();

JsonObject obj = new JsonObject();

obj.addProperty("type", entry.getType().getName());
obj.addProperty("cite_key", entry.getCiteKeyOptional().orElse(""));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Difficult decision here. Alternatives are null or to omit the field. It is OK for me to use the emptry string. Seems to be the JavaScript way.

rmoradc and others added 9 commits September 15, 2020 11:02
MindMapNodes no longer keep list of children but have x and y pos. Done to prevent cyclic dependency complications when serialising.
…ature/23-REST-interface

# Conflicts:
#	src/main/java/org/jabref/gui/jabmap/JabMapAction.java
#	src/main/java/org/jabref/logic/jabmap/MindMapManager.java
#	src/main/java/org/jabref/rest/resources/RootResource.java
@rowanh22 rowanh22 closed this Sep 16, 2020
@koppor
Copy link
Collaborator

koppor commented Oct 15, 2020

Follow-up at #76.

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.

3 participants