neo09 / gwt-platform

Automatically exported from code.google.com/p/gwt-platform
0 stars 0 forks source link

Allow PlaceManager to queue requests while it is locked #187

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
A problem we have at the moment is that the PlaceManager simply refuses to 
navigate when a request is sent and it is already locked, silently failing in 
an unexpected way. Instead, we should probably queue the incoming request and 
trigger it when the place manager is unlocked.

This is discussed there:
http://groups.google.com/group/gwt-platform/browse_thread/thread/da22564f0a83b6a
a

Original issue reported on code.google.com by philippe.beaudoin on 1 Sep 2010 at 9:10

GoogleCodeExporter commented 9 years ago
Maybe the queue could be of maximum depth 1. i.e. We should only remember the 
last request? Are there cases where this would fail? It would simplify and 
speed up the code.

Original comment by philippe.beaudoin on 1 Sep 2010 at 9:27

GoogleCodeExporter commented 9 years ago
I agree. There should only be one pending request. More than one will be user 
error. The only thing I can think is that a request is added through delayed 
commands but why?

Original comment by youareh...@gmail.com on 2 Sep 2010 at 5:39

GoogleCodeExporter commented 9 years ago
Is it really the responsibility of GWTP to "replay" a request? Maybe changing 
revealPlace to return a boolean indicating the success or not is enough. The 
"replay" will become the responsibility of the application, not the framework.

If not, the same problem as the current one ("silently failing in an unexpected 
way") will occur when a second request is done.

Or... a new request cancels the previous... but it seems harder to do.

Original comment by olivier....@free.fr on 3 Sep 2010 at 7:40

GoogleCodeExporter commented 9 years ago
This has been implemented, I forgot to close the issue. See the code review:
http://codereview.appspot.com/2135042/

Cancelling is hard as you would need to make sure it can be done in any async 
portion of the navigation process. Some of these are in user code (async calls 
in prepareFromRequest). I think it would need a major overhaul.

I really like the behavior of the deferred navigation right now. You feel like 
the placeManager is servicing your request instead of ignoring it like it did 
before. It works with any kind of navigation, it should even work when editing 
the url manually while the PlaceManager is locked.

The behavior you describe, not sending a request if the app is processing one, 
could easily be done by providing an isLocked() method in PlaceManager. Should 
we do that?

Original comment by philippe.beaudoin on 3 Sep 2010 at 3:28

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
I don't know... I was just thinking what can cause a new request to be queued. 
This could happen when (1) the presenter code must be loaded (code splitting), 
(2) another place must be revealed will processing prepareFromRequest or (3) 
something else.

For (1), we must want to go immediately to the next presenter. The current 
(loading) presenter will be loaded but not used.

For (2), maybe a specific feature must be designed. If we just what the current 
request to end and them show the next one (like the error place), the view of 
the current request will be shown before the next one is, especially if the 
next one must be loaded by code splitting (I didn't view the CR to check this 
problem yet). Maybe having something like returning a new place request from 
prepareFromRequest or something like the Gatekeeper but for validation can be a 
better solution.

For (3)... what this could be?

Note: I didn't read all the group post and the CR. I just want to point some 
potential problems but they can be invalid.

Original comment by olivier....@free.fr on 5 Sep 2010 at 12:41

GoogleCodeExporter commented 9 years ago
I think queue is really for 2. From the post on the mailing list, people had 
problems while reavealing a new place from the method : prepareFromRequest. 

Ex :
You try to load something Async, but you have some validation on parameters 
entered and it fails, you try to show a new place, but you can't, navigation is 
locked. Even Async would have fails, but you could have a deffered command for 
those case.

Original comment by goudreau...@gmail.com on 5 Sep 2010 at 2:54

GoogleCodeExporter commented 9 years ago
The idea of a queue is to solve (2). Incidentally, it also solves any other 
navigation that could occur during asynchronous call (such as the user clicking 
a link).

Here is when asynchronous call can occur during the revelation process:
(1) Code splitting
(2?) In gatekeepers (not supported yet, coming)
(3) Asynchronous call in prepareFromRequest to fetch data before revealing the 
presenter (the new manual reveal mechanism)

I agree with you that (1) is internal to GWTP and we could write code to handle 
it better (i.e switching right away instead of queuing). The current queue 
mechanism does not prevent this, however, so we can still plan to add this 
feature in the future if there is a need for it.

The real problem occurs (2) and (3) since this happens in user code, the 
asynchronous calls can be complex, chained, etc. If we want to support 
cancelling the request, we need to pass a token (the lock?) to the user code 
and make sure all asynchronous call are made via a structure that is aware of 
this token. This way the calls can be cancelled if needed.

-------
Another thought, that I had writing this.

Another way to handle (3) would be for the manualReveal() to NOT reveal the 
presenter if the navigation was cancelled by another incoming presenter. One 
way to do this would need us to pass a token to prepareFromRequest() and 
manualReveal() and the place manager would make sure this token match its 
current navigation token before revealing.

This means the user's asynchronous calls made within prepareFromRequest will 
NOT be cancelled, but the presenter will not be revealed once these calls 
complete.

I like that idea, we could start thinking about it.

Original comment by philippe.beaudoin on 7 Sep 2010 at 12:36