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

Added utility class for loading fxml views #363

Closed
wants to merge 1 commit into from
Closed

Added utility class for loading fxml views #363

wants to merge 1 commit into from

Conversation

mainrs
Copy link
Contributor

@mainrs mainrs commented Mar 5, 2016

Some time ago I created a small utility class that was capable of loading and setting the stage up for a fxml view. It might help out other people so I created a pull request. The code is documented but not the methods itself.

@mainrs
Copy link
Contributor Author

mainrs commented Mar 6, 2016

Oh just forgot to tell you something, I posted a question on the google groups forums. I have a small problem concerning the process of updating a progress bar from other view models (and not only from the view/viewmodel that contains the progress bar).

@manuel-mauky
Copy link
Collaborator

Hi,
many thanks for your PullRequest. I haven't had the time to take a closer look into the code but I will do this within the next week.
And I have seen your Question in the mailing list but again had not the time to give a complete answer yet. Sorry for that :-(

@mainrs
Copy link
Contributor Author

mainrs commented Mar 7, 2016

Just a small question: Is it possible to notify a view from a view model that is not bound to the view? For example, I want to notify the view from the add contact dialog by calling a method from the view model of the about dialog (some extreme example here). Or is this somehow against the design?


public static <V extends FxmlView<VM>, VM extends ViewModel> void load(Class<V> viewClass, Class<VM> viewModelClass,
Stage parentStage, String title, String... css) {
load(viewClass, viewModelClass, parentStage, title, true, (String[]) null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a bug in line 34. (String[])null needs to be css.

@manuel-mauky
Copy link
Collaborator

Hi,
sorry for the delay. I've looked into your code.

Getting a reference to the Stage of a view is a valid requirement.
However there are some reasons to not use the suggested approach of introducing a new interface with a setStage method:

  1. This approach means that your view has another method as part of it's public API which can be misunderstood by other developers that are using such views.
  2. There is another workaround for accessing the stage of the view by using the getScene().getWindow() method of any JavaFX Node in the view.

In mvvmFX we typically have the approach to inject other things that are needed. For example in the View you can inject the ViewModel instance and the ResourceBundle with specific annotations.
If you think that accessing the Stage in a view is a common use case you can create an issue and we can discuss and try to introduce a similar injection functionality for the Stage.

Additionally we should think about the actual use cases for this. The only real usage I can think of is showing dialogs. For this we already have issues: #171, #331. In this process we should could think about a good API for using dialogs with MVVM.

Now to the second part your pull request, the UILoader class.
If I get the idea correctly it's an alternative API or wrapper for the FluentViewLoader that has specific arguments for specific use cases. This can be misleading for users because the similarity between FluentViewLoader and UILoader.
On the other hand most of the additional arguments (like title, resizable, ...) are only used to display the loaded view in a new Stage. It's more like a dialog helper class.
If this is the main purpose we should discuss this use case in it's own issue and find a good API for dialogs, like I said above.

The general idea is ok but instead of creating overloaded methods we should create a fluent API for this use case.

@mainrs
Copy link
Contributor Author

mainrs commented Jun 21, 2016

I agree. Fluent api sounds great. The name was just a place holder for myself.
You asked why I created the class. There is already a helper class in the example application (#171) that is capable of displaying dialogues. The key difference is that those dialogues do not have the primary Stage as their parent, meaning the focus can switch from dialogue (or any other window that has been displayed) to the main application. That is in most cases not desired. For example a "Save File" dialogue before closing an editor. As a developer, if the user wants to close the application and some files are not saved you can display a message that asks the user to save them. You do not want to let the user tinker around with the files while the dialogue us displaying. Another example could be the about dialogue. Showing it should lock the focus but that is probably just my opinion :)
There are usecases where this could be used and to be honest, it doesn't seem unnecessary.

The api that I created was a prototype. I wrote it in less than 5 minutes to make it work. It was more about the idea.

There are probably two ways to implement this:

  1. Create an interface as I did and a helper class capable of injecting the primary stage into the view to create a usable reference.
  2. bind it to the core mvvmFx mechanism and create an annotation (like @InjectPrimaryStage) that can be used in views to get a reference to the stage. That would probably be the best solution imo. It matches the design principles of mvvmFx by using injection in contrast to configuration.

Would really like to hear back :)

@manuel-mauky
Copy link
Collaborator

I totally agree with you that an API for dialogs needs a way of defining modal dialogs. For this we need the primary stage. The helper class in the example app doesn't provide such functionallity as it isn't a general API (yet).
To create a general API for dialogs we have the issue #331 but we hadn't had the time to address this issue yet.

I really appreciate your feedback and help. But first we should discuss a suitable API in the #331 thread. Maybe we can use the existing dialog helper from the example and improve it somehow so that a primary stage can be used?

@mainrs
Copy link
Contributor Author

mainrs commented Jun 21, 2016

Sure, I am fine with that. I will post new ideas on #331 later on today. Gonna take a look at the helper class.

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