Move SettingsDialog and ErrorDialog in jme3-awt-dialogs module #1748

riccardobl · 2022-01-17T08:55:25Z

This PR removes SettingsDialog and ErrorDialog from the internal code, since they do not work properly in some platforms when AWT is initialized in the same thread of GLFW. ErrorDialog also causes issues with mouse release on fullscreen apps in linux.

SettingsDialog is turned into a standalone class for developers that still wish to use it, by doing so we make it easier to replace with something else, eg. a configurator that takes settings from an external launcher with commandline arguments or environment variables

AppSettings settings=new AppSettings(true);
if(SettingsDialog.showDialog(settings)){
  app.setSettings(settings);
  app.start();
}

ErrorDialog is replaced by an error message handler that can be configured by the developer to show/log/send the error message in the most appropriate way.

Eg. reimplementing the ErrorDialog to reproduce the current behavior will look like this:

JmeSystem.setErrorMessageHandler((message)->{
   ErrorDialog.showDialog(message);
});

By default on desktop ErrorDialog is not used and the error is only printed to System.err.
On android the behavior remains the same since the current error reporting is reimplemented using a default error message handler.

riccardobl · 2022-01-18T19:34:48Z

If there is no objection i think this can be merged

stephengold · 2022-01-18T20:47:17Z

I'd like more time to study this please.

stephengold · 2022-01-18T21:39:25Z

This would be much easier to review if it were split into 3 PRs: one for the native libraries, one for the settings dialog, and one for the error dialog. Would you mind splitting it up please?

stephengold · 2022-01-18T21:41:55Z

Also, the dialog changes have major impact so I think they should be discussed at the Hub/Forum before this is integrated.

riccardobl · 2022-01-18T21:47:22Z

I'll remove the native patch from this PRs and commit it directly, since it is a trivial change of the build script.
Error and Settings dialogs are part of the same problem and prevent the engine from working in MacOS and they are already split in different commits.

I don't see this as a major change, since the same functionality is provided with SettingsDialog.showDialog(settings) and ErrorDialog.showDialog(message), but i'll open a thread

stephengold · 2022-01-18T22:04:12Z

I notice that the "Show Setting" checkbox in the test chooser no longer works. Please fix that.

I'll remove the native patch from this PRs and commit it directly, since it is a trivial change of the build script.

Thanks.

Error and Settings dialogs are part of the same problem and prevent the engine from working in MacOS and they are already split in different commits.

OK, I'll try viewing them that way.

I don't see this as a major change, since the same functionality is provided with SettingsDialog.showDialog(settings) and ErrorDialog.showDialog(message)

I disagree. If you won't start the discussion, then I will. But I imagine you could explain the changes better than I.

riccardobl · 2022-01-18T22:39:24Z

I notice that the "Show Setting" checkbox in the test chooser no longer works. Please fix that.

Should be fixed now

I disagree. If you won't start the discussion, then I will. But I imagine you could explain the changes better than I.

I opened a thread here

stephengold · 2022-01-18T22:47:30Z

Should be fixed now

Tested and confirmed.

I opened a thread here

Looks good. Thank you.

stephengold · 2022-01-19T08:19:23Z

I got distracted today and didn't have time to study this PR in depth. Hopefully tomorrow...

stephengold · 2022-01-19T22:08:17Z

I don't understand why the SKIP_SETTINGS variables were removed from TestIssue1120 and TestGImpactShape. How do those changes relate to this PR?

riccardobl · 2022-01-19T22:22:17Z

Isn't SKIP SETTING used to skip the setting dialog there?

stephengold · 2022-01-19T22:46:57Z

Isn't SKIP SETTING used to skip the setting dialog there?

Yes. And?

riccardobl · 2022-01-19T22:50:50Z

If the settings dialog is removed it cannot unskip it

stephengold · 2022-01-20T05:34:23Z

Okay, the removals make sense then.

I'm trying to run apps in jme3-examples on Linux (this branch of JME) and every test so far has failed. Here's a typical crash log:

> Task :jme3-examples:run
Jan 19, 2022 9:31:30 PM com.jme3.system.JmeDesktopSystem initialize
INFO: Running on jMonkeyEngine 3.1.0-SNAPSHOT
 * Branch: macNatives
 * Git Hash: f6d099e
 * Build Date: 2022-01-19
Jan 19, 2022 9:31:30 PM com.jme3.app.LegacyApplication handleError
SEVERE: Failed to create display
java.lang.RuntimeException: Unable to find fullscreen display mode matching settings
	at com.jme3.system.lwjgl.LwjglDisplay.createContext(LwjglDisplay.java:80)
	at com.jme3.system.lwjgl.LwjglAbstractDisplay.initInThread(LwjglAbstractDisplay.java:120)
	at com.jme3.system.lwjgl.LwjglAbstractDisplay.run(LwjglAbstractDisplay.java:221)
	at java.lang.Thread.run(Thread.java:748)

[JME ERROR] Failed to create display
RuntimeException: Unable to find fullscreen display mode matching settings
Jan 19, 2022 9:31:30 PM com.jme3.app.LegacyApplication handleError
SEVERE: Uncaught exception thrown in Thread[jME3 Main,6,main]
java.lang.IllegalMonitorStateException
	at java.lang.Object.notifyAll(Native Method)
	at com.jme3.system.lwjgl.LwjglAbstractDisplay.initInThread(LwjglAbstractDisplay.java:135)
	at com.jme3.system.lwjgl.LwjglAbstractDisplay.run(LwjglAbstractDisplay.java:221)
	at java.lang.Thread.run(Thread.java:748)

[JME ERROR] Uncaught exception thrown in Thread[jME3 Main,6,main]
IllegalMonitorStateException

Exception: java.lang.NullPointerException thrown from the UncaughtExceptionHandler in thread "jME3 Main"

Ali-RS · 2022-01-20T08:51:21Z

Running on jMonkeyEngine 3.1.0-SNAPSHOT

Why 3.1?

stephengold · 2022-01-20T09:23:18Z

Why 3.1?

I noticed that, too. I suppose that might be because Riccardo's fork lacks any recent build tags:

sgold:~/Git/riccardo/jmonkeyengine$ git tag
3.1-github-tag-test
3.1-snapshot-test
3.1-snapshot-test-2
3.1-snapshot-test-3
3.1-snapshot-test-4
test123
test124
v3.1-alpha1
v3.1.0-alpha2
v3.1.0-alpha3
v3.1.0-alpha4
sgold:~/Git/riccardo/jmonkeyengine$

The branch of "macNatives" from "master" took place less than a week ago.

riccardobl · 2022-01-20T11:03:49Z

Somehow it seems the window is launcher fullscreen

stephengold · 2022-03-25T01:00:56Z

I'm interested in writing a new version of Settings Dialog that doesn't use AWT/Swing.

@riccardobl: Instead of removing/disabling the dialogs, would you consider revising this PR so it's not a breaking change?
In other words, add the hooks to replace Settings Dialog and Error Dialog with alternatives, but keep the current dialogs as the defaults on all platforms.

riccardobl · 2022-03-25T15:13:57Z

I am planning to move the dialogs on a jme3-awt-dialogs module that can be replaced with a different module for different platforms or left out for no dialogs. Would that work?

stephengold · 2022-03-25T16:46:16Z

I think it would.

…alogs

riccardobl · 2022-04-03T13:47:25Z

Done.
I've moved the awt dialogs into the optional jme3-awt-dialogs module and added a "settings handler" in JmeSystemDelegate to handle the initial configurations.

Both error and settings handlers will search by default the classcom.jme3.system.JmeDialogsFactoryImpl that implements JmeDialogsFactory. If found they will call JmeDialogsFactoryImpl.showErrorDialog() and JmeDialogsFactoryImpl.showSettingsDialog(), if not found the settings handler will do nothing and the error handler will print the error message to System.err.

I've implemented the JmeDialogsFactoryImpl in jme3-awt-dialogs to keep the current behavior when the new module jme3-awt-dialogs is included as dependency.
To implement new dialogs the process is to create a new module (eg. jme3-fx-dialogs) and the class com/jme3/system/JmeDialogsFactoryImpl that implements JmeDialogsFactory inside the module.

To reduce the complexity and to have the entire logic selfcontained into the default handlers and the module i made so that a new instance of JmeDialogsFactoryImpl is created everytime a dialog is shown, it shouldn't be a problem, but if proven otherwise i will figure out a different approach.

riccardobl · 2022-04-04T08:54:21Z

I'm interested in writing a new version of Settings Dialog that doesn't use AWT/Swing.

@stephengold let me know if this meets the requirements for the new dialogs

stephengold · 2022-04-05T21:15:40Z

I'll study it. Thanks!

stephengold · 2022-04-11T06:11:42Z

I've begun studying this PR. So many levels of indirection!

I understand correctly, I can create a new library with its own version of com.jme3.system.JmeDialogsFactoryImpl, and applications including that library will automatically use it to generate settings dialogs and error dialogs as needed. That seems sufficient for what I'm interested in doing.

By the way, something I noticed: SimpleApplication.start() is still invoking the deprecated showSettingsDialog() method in JmeSystem. I guess it ought to directly invoke handleSettings() instead?

riccardobl · 2022-04-11T07:12:04Z

I understand correctly, I can create a new library with its own version of com.jme3.system.JmeDialogsFactoryImpl, and applications including that library will automatically use it to generate settings dialogs and error dialogs as needed. That seems sufficient for what I'm interested in doing.

Yes, that's correct.

By the way, something I noticed: SimpleApplication.start() is still invoking the deprecated showSettingsDialog() method in JmeSystem. I guess it ought to directly invoke handleSettings() instead?

Yes handleSettings is the new method.

If you are happy with this, i will fix the conflicts and remove the deprecated calls so that it can be merged.

stephengold · 2022-04-11T21:08:03Z

In my opinion, this PR isn't ready to be merged yet. At the very least, it needs javadoc and reformatting.

riccardobl · 2022-05-24T11:12:46Z

Added the missing documentation.
Should i refactor this to use a system similar to what @Ali-RS described in this thread https://hub.jmonkeyengine.org/t/buffer-allocator-implementation-set-by-lwjglcontext-gets-ignored-sometimes/45594/9?u=riccardoblb ?
I.e. using config files

Ali-RS · 2022-05-24T18:15:41Z

I.e. using config files

IMHO, the current implementation is fine, I believe no need to use a config file in this case.

jme3-android/src/main/java/com/jme3/system/android/JmeAndroidSystem.java

jme3-ios/src/main/java/com/jme3/system/ios/JmeIosSystem.java

stephengold · 2022-12-07T19:50:38Z

I'd like to integrate this PR. However I can't figure out how to do so using the mechanisms built into GitHub. My plan is to integrate the diffs into a new branch in the main repo, but I'm open to other suggestions on how to proceed.

MeFisto94 · 2022-12-07T19:56:52Z

The alternative would be to locally rebase the macNatives branch onto master and then force-push (or merge master into that branch and regularly push).
Do not copy over the files, though, as that conflicts may be meaningful.

pspeed42 · 2022-12-07T21:36:28Z

Force pushing messes up anyone who has the repository checked out. Better to do things a different way.

MeFisto94 · 2022-12-07T21:47:07Z

anyone who has this feature branch checked out (that is actually even on the fork repo).
Could also rebase and push onto this repo, however.

stephengold · 2022-12-07T23:30:00Z

The alternative would be to locally rebase the macNatives branch onto master

I tried that and ran into difficulties that I didn't know how to handle.

Please @MeFisto94, if you know how to do this right, please do so!

MeFisto94 · 2022-12-08T10:37:20Z

Done in https://github.com/jMonkeyEngine/jmonkeyengine/tree/pr/1748
Sadly I can only change this PR's target branch not the source branch.

stephengold · 2022-12-08T22:14:17Z

Very neat. I'll open a new PR for the changes.

stephengold · 2022-12-08T23:06:00Z

Done! Integrated into "master" branch at 9e54d44
Thanks to @MeFisto94

riccardobl force-pushed the macNatives branch 4 times, most recently from 1ca275b to 92291b6 Compare January 17, 2022 10:09

stephengold added this to the Future Release milestone Jan 18, 2022

riccardobl force-pushed the macNatives branch from 92291b6 to d6a9bc5 Compare January 18, 2022 21:55

riccardobl changed the title ~~Add Apple M1 natives. Remove SettingsDialog and ErrorDialog~~ Remove SettingsDialog and ErrorDialog Jan 18, 2022

Refactory Settings/Error dialogs in JmeDialogsFactory and jme3-awt-di…

6ee07d8

…alogs

riccardobl force-pushed the macNatives branch from f6d099e to 6ee07d8 Compare April 3, 2022 13:26

riccardobl changed the title ~~Remove SettingsDialog and ErrorDialog~~ Move SettingsDialog and ErrorDialog in jme3-awt-dialogs module Apr 3, 2022

riccardobl added 3 commits April 4, 2022 09:34

Merge branch 'master' into macNatives

429e973

add build.gradle

971cbdd

Add copyright headers

3bec61a

Fix formatting and documentation

431d7f7

Ali-RS reviewed May 24, 2022

View reviewed changes

jme3-android/src/main/java/com/jme3/system/android/JmeAndroidSystem.java Show resolved Hide resolved

Ali-RS reviewed May 24, 2022

View reviewed changes

jme3-ios/src/main/java/com/jme3/system/ios/JmeIosSystem.java Show resolved Hide resolved

stephengold mentioned this pull request Dec 8, 2022

move SettingsDialog and ErrorDialog to new jme3-awt-dialogs module #1876

Merged

stephengold closed this Dec 8, 2022

stephengold removed this from the Future Release milestone Oct 22, 2024

Move SettingsDialog and ErrorDialog in jme3-awt-dialogs module #1748

Move SettingsDialog and ErrorDialog in jme3-awt-dialogs module #1748

Conversation

riccardobl commented Jan 17, 2022 • edited Loading

riccardobl commented Jan 18, 2022

stephengold commented Jan 18, 2022

stephengold commented Jan 18, 2022

stephengold commented Jan 18, 2022

riccardobl commented Jan 18, 2022 • edited Loading

stephengold commented Jan 18, 2022 • edited Loading

riccardobl commented Jan 18, 2022

stephengold commented Jan 18, 2022

stephengold commented Jan 19, 2022

stephengold commented Jan 19, 2022 • edited Loading

riccardobl commented Jan 19, 2022

stephengold commented Jan 19, 2022

riccardobl commented Jan 19, 2022

stephengold commented Jan 20, 2022

Ali-RS commented Jan 20, 2022 • edited Loading

stephengold commented Jan 20, 2022

riccardobl commented Jan 20, 2022

stephengold commented Mar 25, 2022

riccardobl commented Mar 25, 2022 • edited Loading

stephengold commented Mar 25, 2022

riccardobl commented Apr 3, 2022 • edited Loading

riccardobl commented Apr 4, 2022

stephengold commented Apr 5, 2022

stephengold commented Apr 11, 2022

riccardobl commented Apr 11, 2022

stephengold commented Apr 11, 2022

riccardobl commented May 24, 2022

Ali-RS commented May 24, 2022 • edited Loading

stephengold commented Dec 7, 2022

MeFisto94 commented Dec 7, 2022

pspeed42 commented Dec 7, 2022

MeFisto94 commented Dec 7, 2022

stephengold commented Dec 7, 2022

MeFisto94 commented Dec 8, 2022

stephengold commented Dec 8, 2022

stephengold commented Dec 8, 2022

riccardobl commented Jan 17, 2022 •

edited

Loading

riccardobl commented Jan 18, 2022 •

edited

Loading

stephengold commented Jan 18, 2022 •

edited

Loading

stephengold commented Jan 19, 2022 •

edited

Loading

Ali-RS commented Jan 20, 2022 •

edited

Loading

riccardobl commented Mar 25, 2022 •

edited

Loading

riccardobl commented Apr 3, 2022 •

edited

Loading

Ali-RS commented May 24, 2022 •

edited

Loading