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] Sharelatex Integration #2866

Closed
wants to merge 44 commits into from
Closed

[WIP] Sharelatex Integration #2866

wants to merge 44 commits into from

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented May 25, 2017

Sharelatex Integration. Fixes #156.

Status:
TODO:

  • GUI shows double entries on second connect
  • Fix problems on error requires restart
  • Add status messages
  • Fix problems with Accept changes dialog
  • 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?

System.out.println("LastUpdated " + lastUpdated);
System.out.println("Owner" + owner);

ShareLatexProjectViewModel model = new ShareLatexProjectViewModel(id, name, owner, lastUpdated);
Copy link
Member Author

Choose a reason for hiding this comment

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

@tobiasdiez Could you please have a look if I did this javafx stuff right?
First a login dialog is shown (that part works) and on successful connect you get a list of projects which I want to show then in another dialog in a table
My current question is: I am not sure how and where to pass a list of projects (the view model class) to the other dialog/view.

Copy link
Member

Choose a reason for hiding this comment

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

A few points:

  • This code here looks like it mostly belongs to the logic package.
  • I would create a logic class that provides a convenient interface for the ShareLatex integration (e.g. methods like login(username, password) and getProjects). Lets call this class ShareLatexManager for the moment.
  • This manager class can be initialized in https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/gui/DefaultInjector.java#L25 and then injected into each controller that needs access to the sharelatex functionality, for example similar to the journal abbreviation loader
    @Inject private JournalAbbreviationLoader journalAbbreviationLoader;
  • So you see that in JavaFX you almost never want to pass something directly to a new view.
  • The purpose of view models is to enrich or format the backend data in a nice way to display them directly in the GUI. For example, if one of your sharelatex projects have a property dateCreated but you only want to display the year of creation that the ProjectViewModel only contains yearCreated. But most of the work should really be done in the logic.

Hope this is helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, got it to work now. By the way. Is is okay to fill the ProjectViewModel in the logic or does this violate the gui -> logic dependency?
So should I create a model-container and use the ViewModel separately?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you have a model class (Project) in the model that is used in the logic, and then a simple wrapper (ProjectViewModel) around it for the gui.

@@ -77,6 +78,15 @@ private Node createSourceEditor(BibEntry entry, BibDatabaseMode mode) {
LOGGER.debug("Incorrect entry", ex);
}

codeArea.richChanges().subscribe(x -> {
Copy link
Member Author

Choose a reason for hiding this comment

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

@tobiasdiez I now found a way to get position information when some data is changed in the code editor, e.g. exactly the information I would need for the sharejs thing (I need the range information (e.g. at position 3 the character x was deleted)

Two problems:

  1. This only applies to one entry - whereas I would need a similar function for the whole database content
    The problem is that a simple value changed event is not sufficient, because there I only get the old and new value.
    However, there is something like https://docs.oracle.com/javase/8/javafx/api/javafx/scene/control/TextFormatter.Change.html
    which applies for any controls that have TextInput

  2. This even only fires when I change sth in the code editor and I am not sure if it would be fired if I edit just the author.

Maybe you have some idea?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is a good idea if the sharelatex stuff depends on the entry editor. We have a text comparison engine that is used in the merge dialog and directly gives you the information you need (what characters changed at which position).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is outdated,I decoupled this from the entry editor a while ago

}

@Subscribe
public void listenToSharelatexEntryMessage(ShareLatexEntryMessageEvent event) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@koppor Here I write the new content to the file

Copy link
Member Author

@Siedlerchr Siedlerchr Oct 24, 2017

Choose a reason for hiding this comment

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

@tobiasdiez Here I receive the whole remote bibtex db as string and overwrite the current active database.

My future vision would be: Apply delta changes (lines/cols-diff) from remote to local bibtex database.
But that is still a bit more complicated, so I first want to get the bibtex string version to work as a basis

}

//Actual response handling
private void parseContents(String message) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@koppor Here I parse the responses I get from the server

});
}

public void sendNewDatabaseContent(BibDatabaseContext database) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@koppor Here the changes are sended, the bibtexdb is written into a string and then the changes are calculated

return "";
}

public List<SharelatexDoc> generateDiffs(String before, String after) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@koppor This is what I do for generating the diffs for sending

@koppor
Copy link
Member

koppor commented Oct 19, 2017

I had a phone call with @Siedlerchr some weeks ago. I pointed him to the user experience of the shared database feature. Especially the conflict handling. It seems not be as easy as expected to switch to the behavior, but we agreed that it is worth a try to think of it. Personally, I'd like to have a good user experience here, because this feature might be a killer feature of JabRef.

# Conflicts:
#	src/main/java/org/jabref/gui/entryeditor/SourceTab.java
Copy link
Member

@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.

I quickly went through the obvious things.

build.gradle Outdated
compile "org.glassfish.tyrus.ext:tyrus-extension-deflate:1.13.1"

compile "com.google.code.gson:gson:2.8.0"
compile "org.bitbucket.cowwoc:diff-match-patch:1.1"
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need another diff library? Can't we use the diff library already used in the merge entries dialog? It is OK, if we do. Then, please explain it somehow. Preferrably using MADR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is diff library is the same which is used on the server side of sharelatex

build.gradle Outdated
compile "org.glassfish.tyrus.bundles:tyrus-standalone-client:1.13.1"
compile "org.glassfish.tyrus.ext:tyrus-extension-deflate:1.13.1"

compile "com.google.code.gson:gson:2.8.0"
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have another JSON library in JabRef? Maybe via unirest-java?

Copy link
Member Author

Choose a reason for hiding this comment

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

We already have Guava. And gson is a much better json library than the default implementation

List<ShareLatexProject> projects;
try {
ShareLatexManager manager = new ShareLatexManager();
manager.login("http://192.168.1.248", "joe@example.com", "test");
Copy link
Member

Choose a reason for hiding this comment

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

Could you comment where this magic IP comes from?

Copy link
Member Author

Choose a reason for hiding this comment

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

Still an artifact from local tests.

projects = manager.getProjects();
assertFalse(projects.isEmpty());
} catch (IOException e) {
// TODO Auto-generated catch block
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this comment. Please log exceptions. -- In tests: Why not letting the test method throw an exception as we do in other tests, too?

@koppor
Copy link
Member

koppor commented Oct 19, 2017

I have some UI issues:

tool bar

grafik

  • Fix color

connection dialog

grafik

  • What do I need to enter for the server address? Why is there no value suggested? - I think, the value is https://www.sharelatex.com.
  • Position the "Cancel" button more nicely. Maybe let it start at the same column as the text box?
  • After pressing "Login", the button text should change to "Logging in..." and be disabled. Otherwise, I do not know that JabRef is doing something.

Choose Project dialog

grafik

  • The buttons "Synchronize Library" and "Cancel" should be swapped, because of consistency with the former dialog
  • I do not know, what happens. Will it upload my whole library? There should be help text displayed. I would have expected, that JabRef downloads the bib file from the ShareLaTeX folder and stores it locally in a temporary folder not visible to the user. Similar to the shared database feature: I do not have a local copy (unless I want to).
  • After pressing the synchronize button, my already opened library file is opened. Without an indicator that there is a connection. I think, in the SharedDatabase feature, there is an indicator such as [synced].

Tool bar

If I press the lion button again, I am asked to enter the connection details again. How do I know that the current library is synched?

Menu

grafik

Do I really need to press the "Send changes to server" button? Why is there not an automatic sync like in the case of a shared database and PostgreSQL?

@koppor
Copy link
Member

koppor commented Oct 19, 2017

The synchronization has issues with & in the the text content. When I add ISBN 978-0-471-44846-4 and sync the entry, the & in the URL field are escaped:

grafik

@tobiasdiez
Copy link
Member

From an architectural view point it would be very nice to have a common interface that defines the interaction with external databases (i.e. setup the connection/login, bidirectional sync of changes, etc). Then the current sql-based databases are one implementation, while your sharelatex feature is implemented as another implementation. In the best case the gui only works against the interface. In this way you also ensure that the user experience is the same for all the connections to external databases (e.g. don't run into some of the problems @koppor mentions above). I know this is a lot of refactoring work, but in my opinion definitely worth!

@Siedlerchr
Copy link
Member Author

@tobiasdiez The main difference between the shared SQL stuff and Sharelatex is the way the synchro works: The sql stuff works on an per entry base while the sharelatex.com works on a diff level with respect to the whole bibtex code as once. It simply sees the bibtex "file" as a single string. The diff I get from the server reports a line/col diff. There is no concept of entries.

@tobiasdiez
Copy link
Member

tobiasdiez commented Oct 20, 2017

ok, I see, that complicates things. Nonetheless, you need to have a way to convert the changes coming from ShareLatex in a way JabRef understands them (i.e. in terms of BibEntries), right? Or how do you handle conflicts right now? Ideally you would want to reuse the ChangeScanner class that is used from handle external changes to the file.

Anyway, even if you don't have a common interface to handle changes, a common interface for everything else would also already be a huge step into the right direction.

@koppor
Copy link
Member

koppor commented Oct 20, 2017

  • login data should be persisted similar to the shared-datbase feature

@Siedlerchr
Copy link
Member Author

@tobiasdiez atm I do a trick to receive the whole bibtex code, write it to the file and let the change scanner do it's work

@tobiasdiez
Copy link
Member

Ok, but then I don't see the challenge here. In abstract terms, the change notification thus gives you a list of pairs of bib entries <before, after>. This is the same for the sql stuff and for the sharelatex implementation (and even for external changes to the bib file). Thus all these (messy) implementation details can be hidden under exactly such an interface, where you listen for changes in the above form. Do I miss something?

@koppor
Copy link
Member

koppor commented Oct 20, 2017 via email

@koppor
Copy link
Member

koppor commented Oct 20, 2017

  • 404 at the URL should be caught. If I enter https://www.sharelatex.com/ as URL, there should be no 404, but the / be removed. When setting https://www.sharelatex.com as default, this issue would be solved.

* upstream/master:
  Initializing EntryEditor Tabs on focus (#3331)
  Fix #3292: annotations are now automatically refreshed (#3325)
  Change integrity message for names depending on database mode (#3330)
  Fix location bundle with fast access (#3327)
  Small code cleanup
  Fix "path not found" exception
  Small fix in drag and drop handler of linked files (#3328)

# Conflicts:
#	src/main/java/org/jabref/gui/DefaultInjector.java
Improved gui layout consistency
Remove unused exception from shared db
@Siedlerchr
Copy link
Member Author

I worked on the gui and made it more consistent with other dialogs and I added sharelatex.com as default (although you could use your own instance in a docker container)

@koppor
Copy link
Member

koppor commented Oct 23, 2017

In the File menu, only two of the three elements should be highlighted:

grafik

"Disconnect ..." should only be enabled when sync is active.

* upstream/master:
  update mockito-core from 2.10.0 -> 2.11.0 (#3338)
  Remove underscore in Localized messages (#3336)
  Localisation: French: new entries translated (#3337)
  Refactoring: Lazy init of all editor tabs (#3333)
Fix preferences loading
Return false on boolean prefs get default if no entry is found
@Siedlerchr
Copy link
Member Author

@koppor I just pushed a new version which implements some entry merging based on the dialog you love so much.

  1. Sharelatex server send me an update message -> I store the line number
  2. I do my trick to get all entries from sharelatex -> List with entries
  3. I now identify an entry from the line number (if possible)
  4. If line number is part of entry -> I get that entry: If not I dumb the whole content as before -> External Change
  5. Check if I have an entry with that citekey in the local db
  6. If the entries do not equal I show the Merge Entries dialog.
  7. Merged entries are stored locally

It works if you don't type too fast on the sharelatex server side.
Otherwise you will get that Merge Dialog everytime someone types a char and waits some time at the server side (as the update from the server is sent immediately)

@koppor
Copy link
Member

koppor commented May 11, 2018

Summary of JabCon discussion:

  • Multiple users should be supported "seamlessly"
  • If user A and B work on the bibfile, a modification by user A on a bibtex entry B1 should not trigger any "changes" dialog if user B did not do any changes.
  • A "Merge Entries" dialog should popup if users A and B modified the same entry.

How this works can be tried out using the Shared SQL Database Feature of JabRef, where @obraliar and me worked hard to have a very nice user experience.

Technically, this is not that easy to achieve. However, users will be happy, if it "just works".

At each instance of JabRef:

  • Local state L is known
  • Remote state R is known
  • In case an update is made remotely, the diff can be applied to R.
    So R' is known. Based on R and R', the entries E where a change was made, are known.
    Based on E, changes can be applied to L.
    If in L the entry was not modified: just apply the update.
    If in L the entry was modified: Start the merge entries dialog.

@tobiasdiez
Copy link
Member

What's the status here?

@Siedlerchr
Copy link
Member Author

Works theoretically, but needs some handling of the merge stuff.

@koppor
Copy link
Member

koppor commented Oct 10, 2018

We discussed the usability issues at JabCon 2018.

Meanwhile, it seems my requirements on usability and preventing huge "WTFs" of users are too high. I was hoping that @obraliar would step in as he also implemented the shared-DB-support for JabRef. Maybe #hacktoberfest could motivate him?

My personal last resort is a hacking session at JabCon 2019.

koppor and others added 8 commits August 25, 2019 03:49
# Conflicts:
#	build.gradle
#	src/main/java/org/jabref/gui/DefaultInjector.java
#	src/main/java/org/jabref/gui/JabRefFrame.java
#	src/main/java/org/jabref/gui/shared/ConnectToSharedDatabaseDialog.java
#	src/main/java/org/jabref/gui/shared/MergeSharedEntryDialog.java
#	src/main/java/org/jabref/logic/shared/DBMSConnectionProperties.java
#	src/main/java/org/jabref/model/database/event/CoarseChangeFilter.java
#	src/main/java/org/jabref/preferences/JabRefPreferences.java
#	src/main/java/org/jabref/preferences/PreferencesService.java
* upstream/master:
  remove wrong dependencies
  add missing dependencies
TODO: fix db writers and get string output back
TODO: Fix merge dialog
@koppor koppor self-assigned this Aug 25, 2019
@koppor koppor added the status: waiting-for-feedback The submitter or other users need to provide more information about the issue label Aug 25, 2019
@tobiasdiez
Copy link
Member

What's the status of this PR? Is somebody actively working on it? If not I would close it for now and reopen as soon as somebody has the time to continue with it.

@koppor koppor added freeze hacktoberfest Hacktoberfest tag. See https://hacktoberfest.digitalocean.com/faq/ for details. and removed status: waiting-for-feedback The submitter or other users need to provide more information about the issue labels Oct 3, 2019
@koppor koppor closed this Oct 3, 2019
@koppor koppor mentioned this pull request Nov 24, 2019
5 tasks
@stefan-kolb stefan-kolb deleted the sharelatex branch March 13, 2020 14:22
@koppor koppor restored the sharelatex branch April 6, 2020 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Hacktoberfest tag. See https://hacktoberfest.digitalocean.com/faq/ for details.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Synchronization with Overleaf
4 participants