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

Update MainLoader.java #56

Merged

Conversation

HelgeStenstrom
Copy link
Contributor

The thread that creates a WebBrowserController and a DropboxViewer is named and assigned to a variable. The start() method call is put on a separate line, which can easily be commented out.

If browserThread.start(); is commented out, the thread isn't started, and I don't get the exceptions associated with the JxBrowser.
On the other hand, the Vacuum + Exit fails; it seems to be cleaning something forever, until the application is forced to quit.

Therefore, I haven't commented out start(). But I would like to, because the JxBrowser dependent functionality is not ready yet. It's better to write some unit tests that demonstrate a working WebBrowserController and a DropboxViewer before this thread is re-enabled.

This pull request doesn't change the behavior at all.

The thread that creates a WebBrowserController and a DropboxViewer is named and assigned to a variable. The start() method call is put on a separate line, which can easily be commented out.

If browserThread.start(); is commented out, the thread isn't started, and I don't get the exceptions associated with the JxBrowser.
On the other hand, the Vacuum + Exit fails; it seems to be cleaning something forever, until the application is forced to quit.

Therefore, I haven't commented out start(). But I would like to, because the JxBrowser dependent functionality is not ready yet. It's better to write some unit tests that demonstrate a working WebBrowserController and a DropboxViewer before this thread is re-enabled.
@HelgeStenstrom
Copy link
Contributor Author

It seems that very much is dependent on Main.webBrowser, and if it's null, there will be NullPointerExceptions everywhere. It would be good if as much as possible works both with and without the webBrowser.

Suggestion: Let the type of Main.webBrowser be Optional, and take different actions depending on if it's present or empty. Then one source of NullPointerExceptions is removed.

@goxr3plus
Copy link
Owner

Sorry to keep them not merged so far but i need to carefully test them step by step when i have time .

@HelgeStenstrom
Copy link
Contributor Author

No problem, I don't think you are slow to merge. Having said that, this pull request is completely harmless, I have just introduced a named variable (for the thread).

It's what I actually wanted to do, as the next step, namely not starting the thread (because it doesn't work), which was harmful. And it will take longer time to solve that problem.

If I comment out the thread start, I don't get the error messages to the console that I usually get when I start the player. The problems come later, when I try to exit the player. I think there is a loop that try to close the non-existing browser, and it never terminates.

@goxr3plus
Copy link
Owner

The problem is here will nulls =>

public static void terminateXR3Player(final int exitCode) {
System.out.println("Terminating XR3Player OS->" + OSTool.getOS());
switch (OSTool.getOS()) {
case WINDOWS:
new Thread(() -> {
// Disposing all Browsers...
Main.webBrowser.disposeAllBrowsers();
System.exit(exitCode);
}).start();
break;
case LINUX:
case MAC:
Platform.runLater(() -> {
// Disposing all Browsers...
Main.webBrowser.disposeAllBrowsers();
System.exit(exitCode);
});
break;
default:
System.out.println("Can't dispose browser instance!!!");
break;
}

I should add if (!null) to ignore that WebBrowser instance . But i left it because i was supposingly going to fix the browser issue :) .

@HelgeStenstrom
Copy link
Contributor Author

I did some tests with changing the type of Main.webBrowser into an Optional<>, but it appeared to be complicated.
If you use a value which is Optional, you have to take care of the two cases; either there is a value there, or it's empty. If it's empty, it's similar to the current situation when the value is null.

But there were so many places where decisions needed to be made, and I didn't really know which choice was the right one.

It would have been simpler, if this was not a global value, but passed from caller to called method as an argument, perhaps as dependency injection to constructors. Then the decisions could be taken at an earlier stage in the call change (at least sometimes).

@goxr3plus
Copy link
Owner

I think .

if(Main.webBrowser!=null){
  Main.webBrowser.disposeAllBrowsers(); 
}

will fix the problem .

@HelgeStenstrom
Copy link
Contributor Author

If I do so, and comment out the statement that starts the browser thread (MainLoader.java, line 387 in my branch), I get the following error when I log in:

aug. 29, 2019 6:42:08 EM com.goxr3plus.xr3player.database.DatabaseManager createDatabaseMissingTables
INFO: Creating (if any) missing database tables...
Exception in thread "JavaFX Application Thread" java.lang.NullPointerException
at XR3Player/com.goxr3plus.xr3player.controllers.settings.GeneralSettingsController.lambda$initialize$4(GeneralSettingsController.java:161)
at javafx.base/com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(ExpressionHelper.java:181)
at javafx.base/com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:80)
at javafx.base/javafx.beans.property.BooleanPropertyBase.fireValueChangedEvent(BooleanPropertyBase.java:104)
at javafx.base/javafx.beans.property.BooleanPropertyBase.markInvalid(BooleanPropertyBase.java:111)
at javafx.base/javafx.beans.property.BooleanPropertyBase.set(BooleanPropertyBase.java:145)
at javafx.controls/javafx.scene.control.ToggleButton.setSelected(ToggleButton.java:150)
at XR3Player/com.goxr3plus.xr3player.controllers.settings.ApplicationSettingsLoader.loadApplicationSettings(ApplicationSettingsLoader.java:60)
at XR3Player/com.goxr3plus.xr3player.database.DatabaseManager$DataLoader$1.lambda$call$5(DatabaseManager.java:445)
at javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:428)
at java.base/java.security.AccessController.doPrivileged(AccessController.java:389)
at javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:427)
at javafx.graphics/com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:96)
Exception in thread "JavaFX Application Thread" java.lang.NullPointerException
at XR3Player/com.goxr3plus.xr3player.controllers.settings.GeneralSettingsController.lambda$initialize$4(GeneralSettingsController.java:161)

But it doesn't prevent me from running the application, and you are right, your fix above solves the exit problem.

@goxr3plus
Copy link
Owner

I will remove the JxBrowser on the next version and disable some XR3Player features :(

@goxr3plus
Copy link
Owner

SHOULD i merge this :)?

@HelgeStenstrom
Copy link
Contributor Author

Yes.

@goxr3plus goxr3plus merged commit 7b727c9 into goxr3plus:master Sep 15, 2019
@HelgeStenstrom HelgeStenstrom deleted the namedThreadForBrowserStuff branch September 15, 2019 20:11
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

2 participants