Skip to content
This repository has been archived by the owner on Jun 24, 2021. It is now read-only.

Conversation

Tristan971
Copy link
Contributor

@Tristan971 Tristan971 commented Nov 21, 2018

As per #260, here is a PR to default to xdg-open for document opening on UNIX systems.
It keeps the previous method as fallback in case of failure.

Little implementation note : chose to use try-catch rather than calling which xdg-open as this catches the nonexistence of xdg-open (never seen that for myself though) and also failure of xdg-open itself (in whatever case this might happen).

The only backwards compatibility lost here is if the users/developers were (un)knowingly relying on the lookup order of the browsers and not expecting the default browser to be used (imagining they have multiple browsers installed). I reckon this is acceptable.


As a side-note, while there are no tests relating to that part of the code, I still got test failures relating to prism/gtk.
Would be an interesting topic to investigate given #287 maybe ? (although these warnings don't look like they would be related as they're mostly type casting issues)

> Task :graphics:nativeFontFreetype
/home/tristan/perso/openjdk-jfx/modules/javafx.graphics/src/main/native-font/pango.c: In function ‘Java_com_sun_javafx_font_freetype_OSPango_FcConfigAppFontAddFile’:
/home/tristan/perso/openjdk-jfx/modules/javafx.graphics/src/main/native-font/pango.c:228:73: warning: passing argument 1 of ‘(jboolean (*)(void *, const char *))fp’ makes pointer from integer without a cast [-Wint-conversion]
                 rc = (jboolean)((jboolean (*)(void *, const char *))fp)(arg0, text);
                                                                         ^~~~
/home/tristan/perso/openjdk-jfx/modules/javafx.graphics/src/main/native-font/pango.c:228:73: note: expected ‘void *’ but argument is of type ‘jlong’ {aka ‘long int’}
> Task :graphics:ccLinuxFontPango
> Task :graphics:linkLinuxFontPango
> Task :graphics:nativeFontPango
> Task :graphics:ccLinuxGlassGlass
> Task :graphics:linkLinuxGlassGlass
/home/tristan/perso/openjdk-jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_window_ime.cpp: In member function ‘virtual void WindowContextBase::enableOrResetIME()’:
/home/tristan/perso/openjdk-jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_window_ime.cpp:236:66: error: cast between incompatible function types from ‘int (*)(XIM, XPointer, XPointer)’ {aka ‘int (*)(_XIM*, char*, char*)’} to ‘XIMProc’ {aka ‘void (*)(_XIM*, char*, char*)’} [-Werror=cast-function-type]
         XIMCallback startCallback = {(XPointer) jview, (XIMProc) im_preedit_start};
                                                                  ^~~~~~~~~~~~~~~~
/home/tristan/perso/openjdk-jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_dnd.cpp: In function ‘void process_dnd_source_mouse_release(GdkWindow*, GdkEventButton*)’:
/home/tristan/perso/openjdk-jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_dnd.cpp:827:44: error: cast between incompatible function types from ‘gboolean (*)()’ {aka ‘int (*)()’} to ‘GSourceFunc’ {aka ‘int (*)(void*)’} [-Werror=cast-function-type]
         gdk_threads_add_idle((GSourceFunc) dnd_finish_callback, NULL);
                                            ^~~~~~~~~~~~~~~~~~~
/home/tristan/perso/openjdk-jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_dnd.cpp: In function ‘void process_dnd_source_drop_finished(GdkWindow*, GdkEventDND*)’:
/home/tristan/perso/openjdk-jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_dnd.cpp:939:40: error: cast between incompatible function types from ‘gboolean (*)()’ {aka ‘int (*)()’} to ‘GSourceFunc’ {aka ‘int (*)(void*)’} [-Werror=cast-function-type]
     gdk_threads_add_idle((GSourceFunc) dnd_finish_callback, NULL);
                                        ^~~~~~~~~~~~~~~~~~~
/home/tristan/perso/openjdk-jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_dnd.cpp: In static member function ‘static GdkPixbuf* DragView::get_drag_image(gboolean*, gint*, gint*)’:
/home/tristan/perso/openjdk-jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_dnd.cpp:1116:71: error: cast between incompatible function types from ‘void (*)(gpointer)’ {aka ‘void (*)(void*)’} to ‘GdkPixbufDestroyNotify’ {aka ‘void (*)(unsigned char*, void*)’} [-Werror=cast-function-type]
                                 w, h, w * 4, (GdkPixbufDestroyNotify) g_free, NULL);
                                                                       ^~~~~~
cc1plus: all warnings being treated as errors
cc1plus: all warnings being treated as errors
> Task :graphics:ccLinuxGlassGlassgtk2 FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':graphics:ccLinuxGlassGlassgtk2'.
> java.util.concurrent.ExecutionException: org.gradle.process.internal.ExecException: Process 'command 'gcc'' finished with non-zero exit value 1


@kevinrushforth
Copy link
Collaborator

The above errors you are getting are compilation errors when trying to build the native code for the javafx.graphics module. Are you running gcc 8 by chance? If so, then you are hitting #226.

Copy link
Collaborator

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

I presume you've tested this (manually, since we don't have an automated test for this feature) ?

*/
private void showDocumentUnix(final String uri) throws Exception {
try {
new ProcessBuilder("xdg-open", uri).start();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that I see what this is doing, wouldn't a simpler solution be to just take the original code and add "xdg-open" to the browsers list as the first item? You might still need a try/catch around the execution, so you could try the next item in the list if you got an exception.

In addition to being less code change, it would unify the approach between the case of using xdg-open and the specific browsers in the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed that would be a cleaner way to go about it! Will change to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it. Just slightly reformatted the array declaration as I feel it looks much nicer this way. Let me know if that's fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll take a look when I get a chance.

@Tristan971
Copy link
Contributor Author

(also of course I tried it first)

@Tristan971 Tristan971 force-pushed the JDK-8214069-javafx-use-xdg-open-for-unix-document-opening branch from b34fecf to f35a4df Compare November 21, 2018 09:15
@Tristan971 Tristan971 force-pushed the JDK-8214069-javafx-use-xdg-open-for-unix-document-opening branch from f35a4df to 26fb4c0 Compare November 21, 2018 09:23
@kevinrushforth
Copy link
Collaborator

Tracked in JBS as JDK-8214069.

Copy link
Collaborator

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Looks good with one minor request to remove an unrelated addition of a blank line.

You can just push it as a separate commit with just that one line reverted (best not to squash it which will require a force push). I'll merge it and push to jfx-dev/rt tomorrow.

@johanvos
Copy link
Collaborator

Looks good to me (apart from the unrelated newline)

@kevinrushforth kevinrushforth added the enhancement New feature or request label Nov 29, 2018
@kevinrushforth
Copy link
Collaborator

I'll merge it and push to jfx-dev/rt tomorrow.

Or more accurately, I'll merge it shortly after you push the commit to remove the blank line.

@Tristan971
Copy link
Contributor Author

Removed the linefeed :-)

@kevinrushforth kevinrushforth merged commit 8fab820 into javafxports:develop Nov 30, 2018
@Tristan971 Tristan971 deleted the JDK-8214069-javafx-use-xdg-open-for-unix-document-opening branch November 30, 2018 15:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants