Eli,
Wow, this is incredible detective work. Thanks very much for your
clear overview of the issue.
I gave your patch a detailed review, and it does indeed fix the issue.
I found one thing that I think might be an error, and I made a few
minor changes to improve readability. I've attached a new version of
the patch to the FLUID-1947 issue:
http://issues.fluidproject.org/browse/FLUID-1947
So the one issue that I think might be an error is the removal of the
call to the afterFileComplete event in stopDemo(). I believe we should
still be firing an afterFileComplete event even if there was an error
or the upload was stopped. In part, this will ensure that
SWFUploadManager cleans up the state of the queue correctly. I put the
call back in and tested, and it seems consistent with the behaviour of
the server-backed version. Does this make sense, or am I missing
something subtle here?
Here's a summary of the other changes I made, all of which were minor
readability tweaks:
1. I renamed pauseDemo to stopDemo to be consistent with the our
naming conventions throughout the component.
3. I removed a unnecessary nested if statement in simulateUpload() by
first checking if we're not uploading and bailing immediately.
4. I similarly removed a layer of indenting in finishUploading() using
the same strategy.
Can you take a look at this version of the patch and double-check that
it is correct? If so, go ahead and commit.
Colin
On 12-Dec-08, at 8:33 PM, Eli Cochran wrote:
It took me a long time to get my head around FLUID-1947 but finally
I figured out that what was happening was because we insert a delay
between each file progress event. We do this to simulate what would
happen during and actual upload, and give the user a chance to
respond to the behavior of the component in a simulated upload.
What was happening was that between the moment that we queued up the
next progress and the time that the progress actually happened, the
user could click the Stop Upload button thus firing a bunch of other
events. Depending on the timing of the click, different odd things
would happen.
So instead of doing:
check if we can progress
set next progress on the timer
timer fires next progress
next progress
We need to
check if we can progress
set next progress on the timer
timer fires next progress
check if we can progress
next progress
I also removed the delay on finishUploading because this was another
place where the user could slip an event in, and it wasn't really
necessary for the simulation. At the point that the finishUpload
fires, we should not wait but start in immediately into the next file.
And I switched the code to use that.queue.isUploading instead of
that.demoState.shouldPause which meant that pauseDemo could be
simplified a little bit and we're using a consistent variable to
checking the state of the upload.
Obviously this needs a very detailed review.
- Eli
<FLUID-1947.b.patch>
. . . . . . . . . . . . . . . . . .
.
Eli Cochran
user interaction developer
ETS, UC Berkeley
---
Colin Clark
Technical Lead, Fluid Project
Adaptive Technology Resource Centre, University of Toronto
http://fluidproject.org
_______________________________________________________
fluid-work mailing list - [email protected]
To unsubscribe, change settings or access archives,
see http://fluidproject.org/mailman/listinfo/fluid-work
