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

Add awt native open commands #7037

Merged
merged 6 commits into from
Oct 28, 2020
Merged

Add awt native open commands #7037

merged 6 commits into from
Oct 28, 2020

Conversation

LyzardKing
Copy link
Collaborator

@LyzardKing LyzardKing commented Oct 23, 2020

Fixes flathub/org.jabref.jabref#3
As discussed in gitter the confined releases do not play nicely with xdg-open.
The flatpak fails directly, and the snap asks for confirmation every time.
If we introduce awt.open it works as intended with native commands/windows.
It works in confined and unconfined releases.

At the moment this is a draft pending further tests

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@LyzardKing LyzardKing marked this pull request as draft October 23, 2020 14:53
@Siedlerchr
Copy link
Member

As suspected, you need to add an annotation to the class

java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'no classes that are not annotated with @AllowedToUseAwt should access classes that reside in a package 'java.awt..'' was violated (3 times):
Method <org.jabref.gui.desktop.os.Linux.lambda$nativeOpenFile$0(java.lang.String)> calls method <java.awt.Desktop.getDesktop()> in (Linux.java:29)
Method <org.jabref.gui.desktop.os.Linux.lambda$nativeOpenFile$0(java.lang.String)> calls method <java.awt.Desktop.open(java.io.File)> in (Linux.java:29)
Method <org.jabref.gui.desktop.os.Linux.nativeOpenFile(java.lang.String)> calls method <java.awt.Desktop.isDesktopSupported()> in (Linux.java:25)
at org.jabref.architecture.MainArchitectureTests.doNotUseJavaAWT(MainArchitectureTests.java:75)

@LyzardKing
Copy link
Collaborator Author

Added.
However if I run from source (via gradlew) it works.
If I run jlink/jpackage it fails.
Is it possible that I have to add the awt package in the jpackage module list?

@Siedlerchr
Copy link
Member

java.awt.desktop is part of java.desktop module and that is included. For me jpackage builds fine in your branch under osx.

@LyzardKing
Copy link
Collaborator Author

The flatpak works with the .tar.gz built with the change.
Unfortunately the snap now fails... if I try to open a linked pdf it fails and prints on screen what I think is the error log I left in the code, but garbled: %d [%thread] %-5level %logger - %msg%n

@Siedlerchr
Copy link
Member

You could try to test if System.err.println or System.out.println produces a better error message

@LyzardKing
Copy link
Collaborator Author

I'll try that. If I were to make a random guess I'd say that the Thread part is failing in the snap..
But I'll test it to be sure

@Siedlerchr
Copy link
Member

Siedlerchr commented Oct 23, 2020

Try replacing the new Thread part with Swingutilities.invokeLater( () - > {
Desktop.open...
})
That dispatches the dialog call to the awt event queue https://stackoverflow.com/a/43766312/3450689

@LyzardKing
Copy link
Collaborator Author

LyzardKing commented Oct 24, 2020

The problem wasn't the thread..
The error is

java.io.IOException: Failed to show URI:file:<filepath>

where filepath is not utf-8 decoded (it shows %20 instead of a space), but that might be fine

I have a few things to test, since it might be simply a missing dependency to add to the snap

@Siedlerchr
Copy link
Member

Siedlerchr commented Oct 24, 2020

Ah I see. I think you need to call toAbsolutePath before Desktop.open(file.getAbsolutePath())

Also seems to be buggy on linux in general:
https://stackoverflow.com/questions/9692331/open-a-path-with-desktop-open-from-java-on-ubuntu-linux

@LyzardKing
Copy link
Collaborator Author

Solved. In the snap I had to set it to use the new gtk-portal.
To avoid issues it is not enabled by default, but with that option it works.
The issue is that now it opens PDFs succesfully, but not URLs.
A file is opened via Desktop.getDesktop().open(new File(filepath))
A url is opened via Desktop.getDesktop().browse(new URI(url))
I need to distinguish a filepath from a url... probably the fact that the filepath will always start with a "/"?

@Siedlerchr
Copy link
Member

Siedlerchr commented Oct 24, 2020

This probably needs some more refactoring.
In JabRefDesktop there is a method called openBrowser which simply calls openExternalFilePlatformIndependent :

private static void openExternalFilePlatformIndependent(Optional<ExternalFileType> fileType, String filePath)
throws IOException {
if (fileType.isPresent()) {
String application = fileType.get().getOpenWithApplication();
if (application.isEmpty()) {
NATIVE_DESKTOP.openFile(filePath, fileType.get().getExtension());
} else {
NATIVE_DESKTOP.openFileWithApplication(filePath, application);
}
} else {
// File type is not given and therefore no application specified
// Let the OS handle the opening of the file
NATIVE_DESKTOP.openFile(filePath, "");
}
}

I guess it would make sense to implement a method openBrowser in NativeDesktop interface as default method, e.g.

 default void openBrowser(String url) throws IOException   {
        this.openFile(url, "");
    }

Then in the LinuxDesktop class you can simply overide the implementation.
And then simply change the calls from openBrowser in JabRefDesktop to NativeDesktop.openBrowser

@LyzardKing
Copy link
Collaborator Author

The confined packages do not support the browse function (to open URLs). 👿 😡
So to avoid more refactoring (for now, it might be something to revisit in the future) I left the single nativeOpenFile function, that on a specific exception will revert to "xdg-open", since that works as intended for URLs.

@LyzardKing
Copy link
Collaborator Author

Try reverting to a thread to avoid incurring in the unittest for swing dependency.

@Siedlerchr
Copy link
Member

Thanks for the work, if it works now it should be good

@LyzardKing LyzardKing marked this pull request as ready for review October 25, 2020 16:42
@LyzardKing
Copy link
Collaborator Author

It should be good to go, I didn't end up doing the refactoring, although I think that in the future it might be a good step.
Maybe we can wait to see if javafx ends up implementing some of the native stuff.
I'll just run another snap build to verify that it still works with the Thread (it should).

@LyzardKing
Copy link
Collaborator Author

It works.
@Siedlerchr Should I revert the logs to LOGGER.debug even if they don't work in the snap, remove them directly or leave them as println?

@Siedlerchr
Copy link
Member

Regarding the thread, I think it would make sense to use our JabRefExecutorService instead, because it is based on a ThreadPool and will stop all running threads when you exit Jabref.

JabRefExecutorService.INSTANCE.execute( () -> { 
 // put the code  from inside the new Thread here 
 });

Regarding the Logger output, does not make sense to use them if they don't work correctly.

@Override
public void openFile(String filePath, String fileType) throws IOException {
Optional<ExternalFileType> type = ExternalFileTypes.getInstance().getExternalFileTypeByExt(fileType);
String viewer;

if (type.isPresent() && !type.get().getOpenWithApplication().isEmpty()) {
viewer = type.get().getOpenWithApplication();
ProcessBuilder processBuilder = new ProcessBuilder(viewer, filePath);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Siedlerchr
Can we not remove this part and replace it with
Runtime.getRuntime().exec?
The code would be a lot cleaner..both here and in the openFileWithApplication methods..

Copy link
Member

Choose a reason for hiding this comment

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

This method is used when you have defined a specific app in JabRef (Manage external file types in the Settings dialog -> External programs)

At least we need this StreamGobblers which read the input and error streams, otherwise it could lead to hanging
https://www.infoworld.com/article/2071275/when-runtime-exec---won-t.html

@LyzardKing
Copy link
Collaborator Author

The openFolder function can also be simplified to use the new nativeOpen...
instead of hardcoding some file managers..
It could be another PR though.. so we can first solve the issue of the files not opening..

@LyzardKing
Copy link
Collaborator Author

LyzardKing commented Oct 25, 2020

EDIT: Nope..
It would only be for the default action, so I'm fine with leaving it as it is for now.
I'll look into everything else another time

@LyzardKing
Copy link
Collaborator Author

@Siedlerchr for me this PR is concluded.
It solves the issue. Any other changes will be done in anoter PR in the future.

@Siedlerchr
Copy link
Member

Okay, that sounds good! Just waiting for someone else to take a look as well, so I don't miss anything ;)

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 25, 2020
@Siedlerchr Siedlerchr merged commit bfd9840 into master Oct 28, 2020
@Siedlerchr Siedlerchr deleted the nativeOpenFile branch October 28, 2020 09:34
Siedlerchr added a commit that referenced this pull request Nov 3, 2020
* upstream/master:
  Fix 4040 link
  Bump java-diff-utils from 4.8 to 4.9 (#7061)
  Bump bcprov-jdk15on from 1.66 to 1.67 (#7063)
  Bump checkstyle from 8.36.2 to 8.37 (#7064)
  Bump mockito-core from 3.5.15 to 3.6.0 (#7067)
  Bump controlsfx from 11.0.2 to 11.0.3 (#7066)
  Squashed 'src/main/resources/csl-styles/' changes from 5297abd..5c376b8
  add short date formatter (#7039)
  Add awt native open commands (#7037)
  Add online-link detection to FileFieldParser (#7043)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JabRef does not open files linked to entries
2 participants