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

Implement native messaging #3246

Merged
merged 28 commits into from Apr 26, 2018
Merged

Implement native messaging #3246

merged 28 commits into from Apr 26, 2018

Conversation

tobiasdiez
Copy link
Member

@tobiasdiez tobiasdiez commented Oct 2, 2017

This PR implements Native Messaging so that JabRef is able to talk to Firefox and Chrome (or other browser) extensions #1284.

I tried around a lot with directly implementing native messaging using Java. It worked in principle but was really unstable (everything JabRef wrote in the command line made crashed Firefox). Thus, in the end, I went for a simple powershell script that handles the "native messaging" stuff and converts it to simple command line arguments. As a positive side effect, you can now specify Bibtex code for import directly as a command line argument.

I also took the opportunity to slightly improve the code concerning with the remote listener (the old version didn't allowed arguments with line breaks to be passed).

The installation routine still needs to be automated and this is where I need help. @Siedlerchr @koppor could you please have a look at the following things:

  • Copy jabref.json, JabRef.bat and JabRef.ps1 to the same directory as JabRef.exe.
  • Make sure that the correct file name of the JabRef .jar file is specified in JabRef.ps1 under $jabRefJarFileName.
  • Run REG ADD "HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\NativeMessagingHosts\org.jabref.jabref" /ve /d "C:\path\to\jabref.json" /f from the console (with the correct path to the jabref.json file).

Things that may be improved in the future:


  • 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?) https://github.com/JabRef/help.jabref.org/issues/184
  • If you changed the localization: Did you run gradle localizationUpdate?

@buhtz
Copy link

buhtz commented Nov 15, 2017

Technicaly I understand nearly nothing of what you are doing here. :) But If you need help with testing something I am your man.

@tobiasdiez tobiasdiez added this to the v4.2 milestone Mar 5, 2018
@buhtz
Copy link

buhtz commented Mar 5, 2018

You useing "native messaging via powershell script" now? This doesn't run on Linux I think.

@tobiasdiez
Copy link
Member Author

Yes, for the moment I use a powershell script since the java version was way to unstable. The script itself is very simple https://github.com/JabRef/jabref/pull/3246/files#diff-5ec2abbb42e242133d0073e06942954c and essentially only converts console stream input to a command line parameter for JabRef. So something similar should be possible without problems using a bash script.

A first working prototyp should be ready this week. Stay tuned!

@tobiasdiez
Copy link
Member Author

Sure, all progress is there in form of old commits. The version at b24b599 worked in principle.

@Siedlerchr
Copy link
Member

Maybe we can adapt the install instructions from here?
https://github.com/mdn/webextensions-examples/tree/master/native-messaging

@tobiasdiez
Copy link
Member Author

@Siedlerchr well, this is the current state. The instructions at https://github.com/JabRef/JabFox#installation-and-configuration work but they seem kinda cumbersome and I think the installer should automate them.

@koppor
Copy link
Member

koppor commented Apr 3, 2018

Implemented the three Install4J requirements in HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\NativeMessagingHosts.

@Siedlerchr
Copy link
Member

There is an Architecture Violation:

   MethodSource [className = 'org.jabref.architecture.MainArchitectureTests', methodName = 'firstPackageIsIndependentOfSecondPackage', methodParameterTypes = 'java.lang.String, java.lang.String']
    => org.opentest4j.AssertionFailedError: The following classes are not allowed to depend on javafx ==> expected: <[]> but was: <[src/main/java/org/jabref/logic/remote/server/RemoteListenerServer.java, src/main/java/org/jabref/logic/remote/shared/Protocol.java, src/main/java/org/jabref/logic/remote/client/RemoteClient.java]>

@buhtz
Copy link

buhtz commented Apr 3, 2018

@tobiasdiez You wrote I should try out the new version. I assume you mean I should test it on Debian? This is not possible currently because there is a JabRef-Java-Bug specific to Debian unstable. But I will give you feedback as soon as possible.

@koppor
Copy link
Member

koppor commented Apr 3, 2018

@buhtz Just ensure that openjdk-jre-8 and openjfx are installed. Ensure that java8 is used to start the jar. I assume, you don't want to compile JabRef for yourself (refs koppor#135), but just execute the JAR.

@buhtz You could also try to create a snapcraft image (see #3121). However, this is work in progress and needs patience and testing. The devteam currently has no Ubuntu installation running and the time to get the snapcraft image working.

@buhtz
Copy link

buhtz commented Apr 3, 2018

@koppor Thanks for your hints. But this standard-hints are not working with Debian unstable. The problem is known by the Debianoids and they working on it. It is debian specific and not the fault of JabRef or some of the java components. I don't post bug-numbers here because there are a dozen of them. ;)

@buhtz
Copy link

buhtz commented Apr 3, 2018

@tobiasdiez I am not sure if I understand the steps correct.

I tried to test Windows 7 with JabRef 3.8. Now I see this is not possible of course because your AddOn is based on JabRef 4.x. Currently I have no way to upgrade to 4.x because this would corrupt my library on debian unstable based systems where 4.x is not available currently.

But I have some questions about the test.

Point "Make sure that the correct file name of the JabRef .jar file is specified in JabRef.ps1 under $jabRefJarFileName.". Do you mean line number 14? How should the syntax look like? Could you specifiy this (with an example) for peoople not aware of powershell syntax.
e. g. ?
$jabRefJarFileName = "JabRef-3.8.2.jar"

What is the jabref.bat for?

I have to install the JabFox addon first, right? But it is not available for Firefox Quantum. How should I deal with this?
grafik

@koppor
Copy link
Member

koppor commented Apr 3, 2018

OT: @buhtz I skimmed through the latest reports I thought they are about building JabRef from source not just running the JAR using a JRE8. Especially #893251 reads like the user there tries to compile JabRef instead of just running a (binary) JAR from http://builds.jabref.org/ (e.g., http://builds.jabref.org/nativeMessaging/JabRef--nativeMessaging--latest.jar) by typing java -jar JabRef--nativeMessaging--latest.jar into the command line.

OT2: Reading https://casept.github.io/post/install-snapcraft-on-debian/, I think snapcraft can really be used on Debian unstable.

@buhtz There is no JabRef 8.3. Just test http://builds.jabref.org/nativeMessaging/JabRef--nativeMessaging--latest.jar. I am not sure whether JabRef really corrupts your library. JabRef does not change an entry if you did not do a change on it. I searched our CHANGELOG.md for "breaking", but there is no hit.

@buhtz As far as I understand the implementation, we need your help in converting the .ps script to a .sh script.

@buhtz
Copy link

buhtz commented Apr 3, 2018

@koppor I am using JabRef 3.8. The structure of the bib file is modfied automatic and without notification to the user when you open a with 3.8 generated bib-file witha 4.x JabRef. (what IMO is a bug!) I have to wait until JabRef 4.x is available on Debian unstable. This depends on the package maintainer there.

Snapcraft/Snappy is absolute no choice because of several reasons.

Before I can convert a script I have to understand what is going on here. I have absolutly no idea how this AddOn is working and for what this files are. @tobiasdiez If you really need my help I would suggest you contact me by mail in our native language. ;)

@koppor
Copy link
Member

koppor commented Apr 3, 2018

@buhtz Maybe, you have save actions configured. - JabRef in Debian IMHO depends on #3421, because of Java 9. Seeing the other TODOs, JabRef 4.x in Debian is going to happen in 6 months the earliest.

@tobiasdiez tobiasdiez changed the title [WIP] Implement native messaging Implement native messaging Apr 3, 2018
@tobiasdiez
Copy link
Member Author

@koppor Thanks a lot! You are my personal hero of the day 😃
I tried it out, installed the new version of JabRef and the add-on - bang, that's it. Everything works out of the box. Perfect!

So this PR is now really ready for review

@buhtz Thanks a lot for your offer. I'll write you a mail tomorrow (or the next days, depending on when I find the time). JabRef/JabRef-Browser-Extension#48 contains a bit more information on what has to be done, but is still sparse.

OT: When migrating from 3.8 to 4.2, JabRef indeed changes how groups are stored and, moreover, renames review to comment.

jabref.install4j Outdated
</object>
</void>
<void property="value">
<string>/ve /d "${installer:sys.installationDir}\jabref.json" /f</string>
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 generates a key with a value /ve /d "C:\Program Files\JabRef\jabref.json" /f. However, there actually should be a folder org.jabref.jabref with key Default and only the path as value.
image

It would be nice if you could fix this since this is the last issue that currently prevents merging this PR. Thanks!
See JabRef/JabRef-Browser-Extension#50.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed. I was in a hurry when copy'n'pasting your requirements.

@koppor
Copy link
Member

koppor commented Apr 18, 2018

RegKey fixed.

Current travis error:

Failures (1):
  JUnit Jupiter:RemoteCommunicationTest:commandLineArgumentMultiLinePassedToServer()
    MethodSource [className = 'org.jabref.logic.remote.RemoteCommunicationTest', methodName = 'commandLineArgumentMultiLinePassedToServer', methodParameterTypes = '']
    => Wanted but not invoked:
messageHandler.handleCommandLineArguments(
    ["my message
 and third"]
);
-> at org.jabref.logic.remote.RemoteCommunicationTest.commandLineArgumentMultiLinePassedToServer(RemoteCommunicationTest.java:72)
Actually, there were zero interactions with this mock.
       org.jabref.logic.remote.RemoteCommunicationTest.commandLineArgumentMultiLinePassedToServer(RemoteCommunicationTest.java:72)

@tobiasdiez
Copy link
Member Author

tobiasdiez commented Apr 18, 2018

Thanks a lot for fixing the installer @koppor!

For some reason, some of the remote tests were failing on travis from time to time. I've no idea why and hence disabled these tests on travis now.

@koppor
Copy link
Member

koppor commented Apr 18, 2018

Maybe, because of parallel builds running on the same machine.

@LinusDietz LinusDietz merged commit 534c79d into master Apr 26, 2018
@LinusDietz LinusDietz deleted the nativeMessaging branch April 26, 2018 09:19
@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 26, 2018
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

5 participants