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

Presenter's finish support with callback #273

Closed
wants to merge 1,394 commits into from
Closed

Conversation

reinert
Copy link
Member

@reinert reinert commented Jun 19, 2013

This is the initial implementation of a new feature proposed here: https://groups.google.com/forum/#!topic/gwt-platform/tE2ffH55De8.

I do not expect the request to be accepted at this moment. I just want to show how this feature would function and discuss this issue in a deeper level.

branflake2267 and others added 30 commits April 13, 2013 10:33
Fixing pom messing with Intellij classpath
Conflicts:
	gwtp-carstore/pom.xml

Upgraded cucumber too.
…ration_tests

Conflicts:
	gwtp-carstore/src/test/java/com/gwtplatform/carstore/cucumber/RunCucumberTestInt.java
	gwtp-carstore/src/test/java/com/gwtplatform/carstore/cucumber/application/BasePage.java
	gwtp-carstore/src/test/java/com/gwtplatform/carstore/cucumber/stepdefs/BasicStepdefs.java
	gwtp-carstore/src/test/java/com/gwtplatform/carstore/cucumber/stepdefs/LoginStepdefs.java
	gwtp-carstore/src/test/java/com/gwtplatform/carstore/cucumber/stepdefs/RatingStepdefs.java
	pom.xml
Some cleanup and fixes of integration tests
Conflicts:
	gwtp-core/gwtp-dispatch-client/src/main/java/com/gwtplatform/dispatch/client/DefaultDispatchAsync.java
	gwtp-core/gwtp-mvp-client/src/main/java/com/gwtplatform/mvp/client/proxy/PlaceTokenRegistry.java
	pom.xml
Conflicts:
	gwtp-core/gwtp-dispatch-client/src/main/java/com/gwtplatform/dispatch/client/DefaultDispatchAsync.java
	gwtp-core/gwtp-mvp-client/src/main/java/com/gwtplatform/mvp/client/proxy/PlaceTokenRegistry.java
	pom.xml
@@ -122,6 +122,11 @@ public void show() {
asPopupPanel().show();
}

@Override
public void finish() {
hide();
Copy link
Member Author

Choose a reason for hiding this comment

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

PopupView implements finish() by just hiding.

@reinert
Copy link
Member Author

reinert commented Sep 1, 2013

What a long weekend... :)
I finally got time to implement the basis of this feature.

Highlights:

Added a new important concept inspired by Android: Bundle

Bundle is the package where Presenters put/receive data to/from other Presenters.
For now, it is just a wrapper of a Map. But I suppose it will grow to something more than that.
Bundles are passed though PlaceRequests.
I've introduced it following @christiangoudreau suggestion about not tying presenters one to other.

FinishCallback

FinishCallback is executed on the Presenter's finish method. Conceptually, it is disposable. I.e., the caller Presenter sends a callback to the callee; the callee, executes it and will not call this callback anymore (unless the same caller calls this Presenter passing the same callback again).
It avoids undesired callback execution if another Presenter calls the same target, passing no callbacks.

A Bundle is passed from the calle to the caller on the callback execution, so the target Presenter can send some data to it's caller.
If some callee Presenter wishes to pass data to it's caller on the callback execution it can use the subclass putInBundle(key,value) method.

Both Presenter and View have a no-argument finish method.

For the Presenter, it means the finish of it's cycle of use.
For the View, means discarding itself from the user's screen.

I guess that's all. I've commented the diff in order to clearly expose the ideas as implemented. Next I'll show how this approach avoided the creation of many events.

@reinert
Copy link
Member Author

reinert commented Sep 1, 2013

Suppose the usual case of a crud for some entity, having two presenter/views, one for listing and one for adding/editing.

When listing all entities and triggering one's edition, someone could want to avoid requesting the server unnecessarily for this entity. In order to pass an entity from the listing presenter to the editing, he could implement some EditEntityRequestedEvent, transmitting the data through the event to the target presenter.

As the application grows, having one event for each existing entity could be distasteful.

Also, when in edition mode, and saving the entity, someone could want to avoid requesting the server again for all entities, and just update the new added/edited entity on the existing list. It happens that he is forced to create a EntitySavedEvent, listened only by one handler, in order to receive this saved entity and update the list.

The same distasteful situation happens here. The application could end up overwhelmed by events.

Although, we could address this scenario by resorting to the CustomProxyPresenter feature, just using Bundle associated with the FinishCallback can be simpler, less verbose and more intuitive.

In the former case, we would request the editing presenter passing the entity through the Bundle. The editing presenter would then check if any "entity" data had arrived from the request's bundle, and make use of it.

As to the latter, when the listing presenter was calling the editing one, it would pass a finish callback, expecting the saved entity in the bundle, and executing the logic of updating the list. On the editing presenter's save method, this would put the saved entity in the bundle before finishing.

So, from my limited experience and knowledge with the platform, I see that having such a feature would potentially help the gwtp users like me. I tried to be as simple as possible to explain the practicality of it. If you have any question, critic or suggestion, please do not save. These are forming ideas.

@kuhnroyal
Copy link
Contributor

I like it, didn't look at the code yet.
With the HTML5 History API the bundle could be used as data when calling pushState, just an idea.

@christiangoudreau
Copy link
Member

How is it different from onHide and onUnbind?

@christiangoudreau
Copy link
Member

I mean, onUnbind is already used somehow like your finish method. Then doing all cleanup necessary (bound events are already cleaned up) then calling the view.

No events involved and it is quite straightforward. As for the view, once it's removed from the slot it should be removed from memory by the garbage collector as nothing in it is attached to the dom and that only an unused presenter with no reference to anything else is lying around. Although that could be benchmarked.

@reinert
Copy link
Member Author

reinert commented Sep 11, 2013

I must have expressed myself badly. Instead of a lifecycle method, it would be better faced as just an general activity's finalizing method.

What the finish method (for presenter) does, is just trigger the closing process of the activity (presenter-view).

In practice it will hide current presenter and display the previous visible presenter.

This design is based on Android. I don't know if GWTP has this already implemented. I tried using the removeFromSlot methods, but they require the parent presenter to be injected into the child presenter. I'm targeting a generic way of finalizing current presenter and redirect to previous one.

@christiangoudreau
Copy link
Member

Isn't that equivalent to placemanager.navigateBack()? Which hide the current presenter and goes back to the previous one. I can understand the concept behind Android activities and finish methods, but before introducing something like this, I want to make sure that it is viable into a normalized web flow.

I think that it is not like placemanager.navigateBack() which only hide the current presenter without unbinding it. Then navigate to the previous class.

In any case, I think it could be introduced somewhere in the placemanager instead (placemanger.finalizeCurrentPlace()?). Use the current lifecycle method (unbind).

What do you think?

@reinert
Copy link
Member Author

reinert commented Sep 11, 2013

I agree that we should use existing parts to the fullest. Using PlaceManager to handle Presenter's (or Place?) finalizing sounds reasonable.

I think we should advance to a definitive design concept and balance between Separation of Concerns and practical use.

Are we finalizing a Place or a Presenter, or even an Activity?

I haven't seen the Activity concept in GWTP yet, but maybe we should consider evaluate start thinking about it.

As T. Broyer suggests, using Activities along with Presenters and Views, could promote a better code organization and reusability.

I personally think that using Presenters without Activities makes difficult reusing the Presenter in other platforms. Having an Activity wrapping Presenter and View, performing context specific logic (e.g. lifecycle methods), would help with that.

About performing the finish logic in the unbind method, I agree that it should be done in some lifecycle method, but I'm still in doubt whether use unbind or internalHide. The finishCallback is not meant to be expensive. Instead, it should be a quick and practical way of performing some logic after a called Presenter is finalized (be it unbound or just hidden). Also, it's mandatory to discard the finishCallback after it's execution (in order to avoid undesirable callback executions from other presenter calls).

@reinert
Copy link
Member Author

reinert commented Sep 11, 2013

Although Android considers finishing an activity to destroy it, I don't know if in an ajax application it would be appropriate. In our context this would mean increasing the construction of instances, which could make the application slower (as the finish method is meant to be the first option when navigating back is intended).

@reinert
Copy link
Member Author

reinert commented Sep 11, 2013

As a note, Android doesn't actually destroy the object when the finish method is called. The SO keeps the instance alive in case the user wants to restart it.

@christiangoudreau
Copy link
Member

Using activities alongside presenters. Although it is possible, using the full GWT activities and presenters will lead to some logic being duplicated. If we want to use that pattern, we will need to clearly define it within a GWTP application and remove duplicated lifecycle responsibilities.

The advantage of using the place manager to finish a place (rather then a presenter) is that any presenters could just use it and it would work as expected: Destroy the current place and go back to the previous one.

In Thomas example, he seems to be describing Presenters and PresenterWidgets in GWTP. (Reusable shared components between presenters). It may be the naming we're using (which come from almost 4 years ago) that is confusing or me that didn't understand correctly.

I think that the finish method should only unbind the element. If it does only that, the onUnbind will be called in the same manner that you described. Problem is if you want to do it on both (unbind/hide) where we need to introduce new lifecycle hooks like you described.

@PhilBeaudoin what do you think about this?

@reinert
Copy link
Member Author

reinert commented Sep 12, 2013

I would not say that GWTP implements the MVP in a confusing way. Rather I understand the GWTP implementation as a pure MVP applied upon ajax web applications.

What Google has done, with Android and GWT design solutions, is promoting an specialized way of using the MVP. By resorting to places and activities, we can have more separate and reusable pieces of code.

At my point of view both are manners of using MVP. One uses pure MVP, the other specializes the MVP in order to possibility cross-platform code reuse.

@night199uk
Copy link

This is something I very commonly require. The idea essentially I think is that Presenters are not completely isolated. You have some Parent presenter and a Child presenter[widget] that form one part of some bigger process (I guess this is Activities in GWT). The Parent P[w] and Child P[w] need to communicate events ideally, and since the Child can be instantiated by multiple ParentP[w]'s an Event is not ideal as all Parents will be triggered by that event. This is especially important I think for generifying presenters to reduce boilerplate.

Think about a GenericCRUDPresenter and GenericAddObjectPopupPresenter (with a GenericRestService which doesn't currently seem to work :-)). A GenericAddObjectPresenter would generate an event which would be broadcast to all GenericCRUDPresenters subscribed to the event bus, and the GenericCRUDPresenter would then need to somehow filter the events to the ones it's interested in (ugly).

I guess the difference is between a compile time, broadcast eventBus and a runtime unicast eventBus. I tackle this today by creating interfaces in the Parent and / or Child presenters similar to the UiHandlers functionality, something like this:

Parent presenter declares:
interface GenericPopupFinishedCallbackType {
public void onFinished(Object myObject);
}

E.g. Child presenter declares field/method:
private ParentPresenter.GenericPopupFinishedCallbackType callbackType;

public void addFinishedHandler(ParentPresenter.GenericPopupFinishedCallbackType callbackType) {
this.callbackType = callbackType;
}

void onFinished() {
callbackType.onSuccess(someobject);
}

And parent presenter when creating the sub-presenter or presenterwidget does this:
MyChildPresenterWidget presenterWidget;
presenterWidget.addFinishedHandler(new FinishedCallbackType() {
@OverRide
public void onSuccess(Object myObject) {
// do something if the child presenter was successful
}
/// etc
}
addToPopupSlot(presenterWidget);

This allows me to set up direct and targetted communication between specific instances of presenters, avoiding the event bus and allowing more generic classes without listening to events from all children in the generic parents or sending all child events to all possible parents, or storing a reference to the parent presenter itself in the child. This works well for me but reducing the boiler plate around this would be useful.
I'd love to see this work not just for Finish events, but any kind of events - allowing Presenters to generate specific, targetted events to Observers registered at runtime for Presenter<->Presenter communication.

@olafleur
Copy link
Member

Somebody wants to take the lead to finish this?

@Chris-V
Copy link
Member

Chris-V commented Nov 18, 2014

Largely outdated. If someone wants to push the idea further it can be reopened.

@Chris-V Chris-V closed this Nov 18, 2014
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.