sialcasa / mvvmFX

an Application Framework for implementing the MVVM Pattern with JavaFX
Apache License 2.0
489 stars 105 forks source link

Scope Isolation #355

Closed gtnarg closed 8 years ago

gtnarg commented 8 years ago

If I have a master/detail views that are opened multiple times, how do I separate the scope instances for each?

manuel-mauky commented 8 years ago

For this use case we have the possibility to define a name (as String). Each time you open the view you have to define a unique name. The scopes feature isn't finished yet and we are trying out possible solutions but aren't sure yet, which is the best. If the current implementation doesn't work for your use case we are happy to hear about it and we will try to find a better solution.

gtnarg commented 8 years ago

There are two issues I see with the name approach.

@InjectScope("coolId2") I can't use it as an attribute of the annotation since that can't be dynamic. I would want to generate a new id every time I open a new master view.

ScopeStorage.get(TestScope.class, "cooldId2"); If I do this, I need to compute a different id each time and share it amongst the the currently opened master/detail viewmodels (which is what Scope is trying to accomplish in the first place).

Perhaps when loading a detail view, you could offer an api which references the master view (which in turn could share it's scope object)?

eg. FluentViewLoader.fxmlView(View.class,parentViewInstance).load();

gtnarg commented 8 years ago

An even simpler design would be:

MyScope myScope = new MyScope();
FluentViewLoader.fxmlView(MasterView.class).scope(myScope).load();
FluentViewLoader.fxmlView(DetailView.class).scope(myScope).load();

"myScope" can then be injected directly into any ViewModels.

gtnarg commented 8 years ago

Any other thoughts on this :)

manuel-mauky commented 8 years ago

Sorry for the late response.

Your second proposal with the scope instances beeing passed to the FluentViewLoader looks nice. However there are some questions we would need to answer first. Let's see some example code:

class MasterViewModel implements ViewModel {
    @InjectScope
    private MyScope scope;
    ....
}

class SomeOtherViewModel implements ViewModel {
    @InjectScope
    private MyScope scope;
}

The code looks like there is the same global scope instance injected in both classes. However due to the fact that you passed an existing scope instance to the FluentViewLoader for MasterView, the MasterViewModel would get this existing instance injected instead. Maybe this isn't a problem at all but on the other hand it makes the reasoning about what happens harder, doesn't it?

Another issue: Assume that MasterView.fxml has another SubView.fxml included via fx:include. Which instance should the SubViewModel get injected? Or more general: Should the scope be used for the whole sub-scenegraph? This seems to be useful, however at the moment we have no way of implementing this because due to limitations of the FXMLLoader we can't easily find out whether an FXML file is included into another FXML file.

sideisra commented 8 years ago

Concerning included views: Would it be possible to "activate" a scope via a ThreadLocal for one FluentViewLoader-Session (FluentViewLoader call). The FluentViewLoader would inject the newly created scope for this loading session in every ViewModel it loads no matter if directly or via fx:include. This would give the whole subgraph one scope which is the purpose of scopes as i understand it (at least we have such structures and requirements in our project ;-)).

gtnarg commented 8 years ago

@sideisra - I like that idea :) I was also thinking that the scope could be a node property on the parent that is accessible from the child view - similar idea to the threadlocal - just a different storage location (traversable from the child by the loader).

@lestard - I see where there might be confusion in reasoning about what happens. I think that confusion revolves around the current implementation treating scopes as singletons which is not the purpose of a scope (as far I understand scope). In addition, these scope objects will never get destroyed. If this was a web application, the scope would most likely be tied to a request/response. Since this is a desktop application, I think we need to control the lifecycle of the scope ourselves (similar to the lifecycle of the views themselves).

manuel-mauky commented 8 years ago

I don't get how a ThreadLocal would be any help in this case? Typically all views are loaded in the same JavaFX UI-Thread which means that all views and viewModels would share the same instance of the ThreadLocal.

Ok, first let's figure out what scopes is used for. The purpose of scopes is to share data and communicate between parts of the application (think viewModels) without a tight coupling between these components.

To archive this we need to have a look at two aspects:

  1. Different parts of the application can have different instances of the same scope type at the same time. For example think of a dialog that contains 2 views/viewModels that are using the same scope instance. If we now open two dialogs at the same time we need to have 2 instances of the scope and we need to assign the correct instance of scope to the correct viewmodel.
  2. A scope can has a livecycle. At different places in time there can be different instances of the same scope type. When we open the dialog we like to create a new instance of the scope that should be killed after the dialog was closed.

In some usecases a solution for these aspects would be to define the scope at loading time. Either by passing an existing instance of the scope to the FluentViewLoader like you suggested some comments above. Or by implicitly using a new scope instance on every load.

However there are some situations where this wouldn't be sufficient. Think of this view hierarchie:

We are loading the MainView with the FluentViewLoader. Both SubView1 and SubView2 are using the same scope instance. However at runtime the sub views under PartOne should have another instance of the scope then the sub views under PartTwo. How would you archive this?

The first and hardest task for all of these problems is: How should the API look like for the developer? In PartOne/SubView1, how do you specify that you need the same scope that PartOne/SubView2 has but another instance then PartTwo/SubView1 and PartTwo/SubView2 is using? A new argument for the FluentViewLoader isn't working here.

The idea of the current scopes API is this: The PartOne view can access both SubView1 and SubView2 and call a method and pass a unique String to both subviews. The subviews now can pass this string to their viewModels whichthen can use the string to get the same scope instance. The bad thing here is that the handling of scopes should only be done by the viewModels and not the views.

Another approach that we are thinking of is the following: Before loading a view hierarchie with the FluentViewLoader we could analyse the structure of the FXML files and get an idea of the hierarchy. We could use this information to pass the correct scope instances to the correct viewModels. The problem here is again the question of how the API should look like? How can a ViewModel define, which instance of scope it needs? We have no good answer for this question at the moment.

But again: Many thanks for your suggestions. The current API for the scopes isn't finished but is a work-in-progress. We have added the API to see how it works and what problems are still there. Your feedback and suggestions are very welcome.

sideisra commented 8 years ago

I agree that the PartOne, PartTwo problem is hard, may be it should not be part of the first improvement step of the Scope API. This would need further experience with the feature. I can't remeber a case where this feature is really necessary. Furthermore, PartOne and PartTwo could be loaded manually by two different FluentViewLoader calls.

I think the isolation of two view hierachies loaded in two different FluentViewLoader runs should come first. For this, it would be sufficient to either pass an instance of the scope to the FluentViewLoader or to create a new instance at ervery run (or support both ;-)). The most important part here is that these scopes are isolated and each view hierachie can work with its own.

gtnarg commented 8 years ago

Looking a little more closely at the current implementation, I think much of what we are looking for could be accomplished with a small change to the ScopeStore.

It currently creates and holds a reference to a singleton (which never goes out of scope). Instead, it could be modified to instantiate a new ScopeStore with each FluentViewer#load (or series of calls if you want to share the ScopeStore between loads).

The pattern would essentially be:

try {
    ScopeStore.init(); // instantiate a new ScopeStore for use during the load
    FluentViewLoader.load(view1); // any scope injections would get shared (even to subviews)
    FluentViewLoader.load(view2); 
} finally {
    ScopeStore.reset(); // release the reference
}

Here is how I see this working within the FluentLoader itself

JavaViewStep.load(){
    boolean initialized = ScopeStore.isInitialized();
    if( !intialized ){ // account for nested calls
        ScopeStore.init(); // instantiate a new ScopeStore;
    }
    try {
        JavaViewLoader javaViewLoader = new JavaViewLoader();
        return javaViewLoader.loadJavaViewTuple(viewType, ResourceBundleManager.getInstance().mergeWithGlobal(resourceBundle), viewModel);
    } finally {
        if( !initialized ) {
            ScopeStore.reset(); // release the ScopeStore reference on the outermost call
        }
    }
}
sialcasa commented 8 years ago

I like your approach, maybe we should provide for this transactional scope creation an additional Scope type :

interface TransactionalScope extends Scope

to check during the instantiation of a scope that a transaction (ScopeStore.init()) was started.

gtnarg commented 8 years ago

It would also be helpful if ScopeStore was a pluggable interface (much like dependency injector).

sialcasa commented 8 years ago

We continue this discussion on: https://github.com/sialcasa/mvvmFX/issues/383