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

Scope Isolation #355

Closed
gtnarg opened this issue Jan 25, 2016 · 13 comments
Closed

Scope Isolation #355

gtnarg opened this issue Jan 25, 2016 · 13 comments

Comments

@gtnarg
Copy link

gtnarg commented Jan 25, 2016

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

@manuel-mauky
Copy link
Collaborator

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
Copy link
Author

gtnarg commented Jan 26, 2016

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
Copy link
Author

gtnarg commented Jan 26, 2016

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
Copy link
Author

gtnarg commented Feb 12, 2016

Any other thoughts on this :)

@manuel-mauky
Copy link
Collaborator

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
Copy link
Contributor

sideisra commented Mar 7, 2016

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
Copy link
Author

gtnarg commented Mar 7, 2016

@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
Copy link
Collaborator

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:

  • MainView.fxml
    • PartOne.fxml
      • SubView1.fxml
      • SubView2.fxml
    • PartTwo.fxml
      • SubView1.fxml
      • SubView2.fxml

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
Copy link
Contributor

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
Copy link
Author

gtnarg commented Apr 4, 2016

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
Copy link
Owner

sialcasa commented Apr 7, 2016

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
Copy link
Author

gtnarg commented Apr 11, 2016

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

@sialcasa
Copy link
Owner

We continue this discussion on: #383

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants