Mr. Clark,
Your changes look good. Excellent, in fact.
In thinking about that final afterFileComplete event, I realized that
there is one more minor little problem that can wait until after 0.6.
At the moment when the file errors, there is a call which hides the
total progressor and updates the Total status text. Unfortunately it
does a quick animation to 100% before it hides it even if the upload
was cancelled.
Thanks for the review and the improvements. Since yours is the last
patch, please commit. Although I suppose you should ask the king first.
- Eli
On Dec 13, 2008, at 9:30 AM, Colin Clark wrote:
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
. . . . . . . . . . . . . . . . . .
.
Eli Cochran
user interaction developer
ETS, UC Berkeley
_______________________________________________________
fluid-work mailing list - [email protected]
To unsubscribe, change settings or access archives,
see http://fluidproject.org/mailman/listinfo/fluid-work
