chore: organize GuiUtils and FXUtils into Effects and Dialogs

[skaldarnar]

These two utility classes have the same functionality, so there is no reason why they should be separated. I think all UI/JavaFX related code should live in a ui package, therefore, I'm merging GuiUtils into FXUtils.

[keturn]

Hmm, I'm not really opposed, but making a collection named "Utils" bigger isn't much of a win either.

I notice the things from GuiUtils are all dialogs, and calling those public methods immediately results in user-visible changes happening.

The lone method in FXUtils createScaleTransition, on the other hand, configures a type of animation and returns it without applying it to anything in particular.

They could be split in to things named "dialogs" and "animations" or "effects" or something like that?

[skaldarnar]

They could be split in to things named "dialogs" and "animations" or "effects" or something like that?

Yep, that would also be an option. I was wondering whether all these dialog utils are strictly required or whether we could get rid of some of them... 🤔 What is the idiomatic JavaFX way of handling this?

Maybe the main utility aspect here is running something on the JavaFX event thread, rather than what exactly we are running ... let me check something...

[keturn]

Additional consideration: #571 used FXUtils as a place to put a FxTimer class, adopted from ReactFX. (It seemed like a reasonable idea to copy the single class, instead of depending on the entire library.)

It looks like that is a class, not a static method, so it could stand alone in its own .java file without needing to be put in one of these.

[keturn]

and yes, I also read those dialog methods and thought "are we missing something?"

It does feel like a use case that ought to be handled by some upstream interface already.

[keturn]

The renames here look good. I just want to look up a thing or two related to that runOnEventThread stuff before sending this on.

[skaldarnar]

Sure, please feel free to push changes and merge if you feel save enough about it 😉

[keturn]

Long-term, there are usage patterns here that I think we should get away from. Specifically the blocking calls like future.get() or showAndWait in the application thread.

I guess we do want these to be modal dialogs, and maaaaaaybe that's the way you're supposed to do modal dialogs? but I am suspicious.

But that's not really in scope for now. This PR is a mostly-simple refactor, I just had to look at it a little closer because runOnEventThread is not quite a simple mechanical refactor, and when I started reading it I didn't realize that much of its logic is extracted straight out of chooseDirectoryDialog.

Refactoring approved.

[keturn]

Ah, I have seen functions like this in the API before. But they're in TestFX: WaitForAsyncUtils, and it would seem weird to include TestFX as an implementation dependency instead of a test dependency.

[keturn]

I wondered: What would happen if this if clause weren't here? But I have a horrible suspicion about what the answer is: we'd ask it to "run later", and then move on to the blocking task.get(), and then "later" would never come because we're blocking the application thread.

ughhhhh.

[skaldarnar]

I was wondering about that, too. At some point, we've put this if construct in the main code, and I hope for some reason (maybe the one you described above) 🙈 I did not double-check that, though, and this is probably something we could unit test in the future, too.

[keturn]

and the main reason for this Platform.runLater is that we're using this with Dialog.showAndWait, which says

This method must be called on the JavaFX Application thread. Additionally, it must either be called from an input event handler or from the run method of a Runnable passed to Platform.runLater. It must not be called during animation or layout processing.

[keturn]

and while showMessageDialog has no return value, we're also using this method to remove the duplication in chooseDirectoryDialog. That is where the isFxApplicationThread switch comes from, and that's why we need a Task with a return value.

[skaldarnar]

Yep, that's basically the train of thought here - including the part further above about the dialogs being modal. We had non-modal dialogs in the past, and they kept vanishing behind the launcher window and thus were often missed by users - so far that they kept working with the launcher which was trying to tell that it has crashed 🙈

Maybe we can move to a more modern notification area where these messages show up in a non-blocking way instead. And only use modal dialogs for hard crashes from which we cannot recover.

[keturn]

Suggested change

	logger.warn("Uh oh, something went wrong running this on the event thread.", e);
	logger.error("Uh oh, something went wrong running this on the event thread.", e);