List<T> not performing as expected

[wannabe geek]

I have an form that displays a slideshow of pictures. It uses a
List<PictureFile> to store the slideshow photos, filenames, and options
whether or not a photo should be included in a list to export later. When I
make one or more items of List<PictureFile> included, the GoToNextPicture and
GoToPreviousPicture methods do not work correctly and whenever they attempt
to move to an included picture, they jump around. I suspected something in
List<T>.ElementAt and switched to List<T>[index] but had no luck.

My code: (I trimmed out some irrelevant methods and also lines of code)

//FormMain.cs

using System;
using System.Collections.Generic;
using System.Drawing;
using System.Windows.Forms;
using System.IO;

namespace slideshow
{
public partial class FormMain : Form
{
List<PictureFile> pictures;
PictureFile curFile;
int curFileIndex;

private void GoToNextPicture()
{
if (pictures.Count != 0)
{
curFileIndex += 1;
if (curFileIndex == pictures.Count)
{
curFileIndex = 0;
}
curFile = pictures[curFileIndex];
}
DisplayPicture();
}

private void GoToPreviousPicture()
{
if (pictures.Count != 0)
{
curFileIndex -= 1;
if (curFileIndex == -1)
{
curFileIndex = pictures.Count - 1;
}
curFile = pictures[curFileIndex];
}
DisplayPicture();
}

private void GoToFirstPicture()
{
if (pictures.Count != 0)
{
curFile = pictures[0];
}
DisplayPicture();
}

private void DisplayPicture()
{
if (pictures.Count != 0)
{
pboxDisplay.Image = curFile.Image;
rbInclude.Checked = curFile.Include;
rbInclude.Enabled = true;
rbDontInclude.Checked = !curFile.Include;
rbDontInclude.Enabled = true;
}
else
{
pboxDisplay.Image = null;
rbInclude.Checked = false;
rbInclude.Enabled = false;
rbDontInclude.Checked = false;
rbDontInclude.Enabled = false;
}
if (timer.Enabled)
{
timer.Stop();
timer.Start();
}
}

private void rbInclude_CheckedChanged(object sender, EventArgs e)
{
curFile.Include = rbInclude.Checked;
GoToNextPicture();
}
}
}

//PictureFile.cs

using System.IO;
using System.Drawing;

namespace slideshow
{
class PictureFile
{
FileInfo file;
bool include = false;
Image image;

public PictureFile(string filename, Image image)
{
file = new FileInfo(filename);
this.image = image;
}

public bool Include
{
get
{
return include;
}
set
{
include = value;
}
}

public FileInfo File
{
get
{
return file;
}
set
{
file = value;
}
}

public Image Image
{
get
{
return image;
}
set
{
image = value;
}
}
}
}

Does anyone have an answer to this?
Ask if you have questions.

 

[Peter Duniho]

I have an form that displays a slideshow of pictures. It uses a
List<PictureFile> to store the slideshow photos, filenames, and options
whether or not a photo should be included in a list to export later. When I
make one or more items of List<PictureFile> included, the GoToNextPicture and
GoToPreviousPicture methods do not work correctly and whenever they attempt
to move to an included picture, they jump around. I suspected something in
List<T>.ElementAt and switched to List<T>[index] but had no luck. [...]

Without a concise-but-complete code example, it's hard to know for sure.

But based on the code you did post, I would say that the problem is most
likely because in your DisplayPicture() method, you modify the state of
the rbInclude checkbox, and you have an event handler that when the
state of the rbInclude checkbox is modified, you change the current picture.

Basically, any time the new picture has a different "included" state
than the currently selected picture, the code will implicitly and
iteratively "search" for the next picture that has the same state as the
one just before it (since when the states are the same, the
rbInclude_CheckedChanged() method stops getting called).

IMHO, it's poor UI design for the current picture being displayed to
change just because the "include" checkbox state has been changed. So
the most obvious fix to me is just to remove the GoToNextPicture() call
from your event handler.

Otherwise, you'll have to keep track of the fact that you're
programmatically modifying the checkbox state, and ignore the event
being raised during that time (e.g. keep a bool variable that's set
while your own code is modifying the checkbox state, and reset after
you're done).

Pete

 

[kndg]

Hi,

Here is my observation:

- you are maintaining two states: curFile and curFileIndex.
I would get rid of curFile and just use curFileIndex

- GoToFirstPicture() have a bug.
You need to reset curFileIndex to 0

- be defensive
instead of "if (curFileIndex == pictures.Count)" and "if
(curFileIndex == -1)", I would use "if (curFileIndex >= pictures.Count)"
and "if (curFileIndex < 0)"

- your timer looks suspicious
I wonder what the timer do...

Perhaps a little more code would help.

Regards.

 

[Peter Duniho]

[...]
- be defensive
instead of "if (curFileIndex == pictures.Count)" and "if (curFileIndex
== -1)", I would use "if (curFileIndex >= pictures.Count)" and "if
(curFileIndex < 0)"

Actually, I would use an entirely different approach (also making the
assumption that the OP takes your advice and gets rid of the "curFile"
variable):

private void GoToPicture(int index)
{
if (pictures.Count > 0)
{
curFileIndex = (index + pictures.Count) % pictures.Count;
DisplayPicture();
}
}

private void GoToNextPicture()
{
GoToPicture(curFileIndex + 1);
}

private void GoToPreviousPicture()
{
GoToPicture(curFileIndex - 1);
}

private void GoToFirstPicture()
{
GoToPicture(0);
}

Of course, it's possible that with a simplified implementation as above,
only the first method is really necessary. Just depends on the actual
usage, but the helper methods now do so little that the code is probably
just as maintainable without them.

Pete

 

[kndg]

[...]
- be defensive
instead of "if (curFileIndex == pictures.Count)" and "if (curFileIndex
== -1)", I would use "if (curFileIndex >= pictures.Count)" and "if
(curFileIndex < 0)"

Actually, I would use an entirely different approach (also making the
assumption that the OP takes your advice and gets rid of the "curFile"
variable):

private void GoToPicture(int index)
{
if (pictures.Count > 0)
{
curFileIndex = (index + pictures.Count) % pictures.Count;
DisplayPicture();
}
}

private void GoToNextPicture()
{
GoToPicture(curFileIndex + 1);
}

private void GoToPreviousPicture()
{
GoToPicture(curFileIndex - 1);
}

private void GoToFirstPicture()
{
GoToPicture(0);
}

Of course, it's possible that with a simplified implementation as above,
only the first method is really necessary. Just depends on the actual
usage, but the helper methods now do so little that the code is probably
just as maintainable without them.

Pete

Very nice. You handle the overflow/underflow in a smart way.

 

[Peter Duniho]

Very nice. You handle the overflow/underflow in a smart way.

Thanks. It's worth pointing out that the technique isn't impervious to
misuse. It basically assumes that the input will be no less than
"-count". In this respect, my suggestion is much less fragile than the
original code, but is still more fragile than the suggestion you provided.

I like the simplicity of the code I suggested, but would not argue with
someone who preferred a more robust implementation.

If one is anticipating the methods to be used in a less-constrained way
(i.e. the input can be received from a source outside the control of
that class), your suggestions for the range-checking part would be a
much better choice (than either my suggestion or the original code).

Pete

 

[wannabe geek]

Hi,

Here is my observation:

- you are maintaining two states: curFile and curFileIndex.
I would get rid of curFile and just use curFileIndex

- GoToFirstPicture() have a bug.
You need to reset curFileIndex to 0

I just added curFileIndex to try to fix the bug. It didn't work, and it
looks like I missed a spot. I will remove curFileIndex.

- be defensive
instead of "if (curFileIndex == pictures.Count)" and "if
(curFileIndex == -1)", I would use "if (curFileIndex >= pictures.Count)"
and "if (curFileIndex < 0)"

Good advice. I didn't think it necessary, but I guess it's good practice.

- your timer looks suspicious
I wonder what the timer do...

Switches the slide show picture every x milliseconds.

 

[wannabe geek]

I have an form that displays a slideshow of pictures. It uses a
List<PictureFile> to store the slideshow photos, filenames, and options
whether or not a photo should be included in a list to export later. When I
make one or more items of List<PictureFile> included, the GoToNextPicture and
GoToPreviousPicture methods do not work correctly and whenever they attempt
to move to an included picture, they jump around. I suspected something in
List<T>.ElementAt and switched to List<T>[index] but had no luck. [...]

Without a concise-but-complete code example, it's hard to know for sure.

But based on the code you did post, I would say that the problem is most
likely because in your DisplayPicture() method, you modify the state of
the rbInclude checkbox, and you have an event handler that when the
state of the rbInclude checkbox is modified, you change the current picture.

Basically, any time the new picture has a different "included" state
than the currently selected picture, the code will implicitly and
iteratively "search" for the next picture that has the same state as the
one just before it (since when the states are the same, the
rbInclude_CheckedChanged() method stops getting called).

IMHO, it's poor UI design for the current picture being displayed to
change just because the "include" checkbox state has been changed. So
the most obvious fix to me is just to remove the GoToNextPicture() call
from your event handler.

This is to move to the next picture when the person has decided to include
the picture in the export pictures. They do not need to preview the picture
any longer and wish to see the next one with little effort.

Otherwise, you'll have to keep track of the fact that you're
programmatically modifying the checkbox state, and ignore the event
being raised during that time (e.g. keep a bool variable that's set
while your own code is modifying the checkbox state, and reset after
you're done).

Pete
.

I tried to post all the necessary code, and I believe you found the bug. Now
I need to use a different event for the radiobutttons other than
CheckedChanged. The first one that comes to mind is Click. If you have any
other suggestions let me know.
Thanks!

 

[Peter Duniho]

I just added curFileIndex to try to fix the bug. It didn't work, and it
looks like I missed a spot. I will remove curFileIndex.

No, don't do that. Remove "curFile" as "kndg" suggested.

I still think your problem is with your event handler.

 

[Peter Duniho]

[...]

IMHO, it's poor UI design for the current picture being displayed to
change just because the "include" checkbox state has been changed. So
the most obvious fix to me is just to remove the GoToNextPicture() call
from your event handler.

This is to move to the next picture when the person has decided to include
the picture in the export pictures. They do not need to preview the picture
any longer and wish to see the next one with little effort.

I understand the underlying motivation. That's not the point. What
about the poor user whose actions do not fit into your narrowly-defined
prescription? Maybe someone changes their mind after clicking. Or
maybe they just clicked accidentally?

Note also that the code posted is not consistent with your description.
The current picture selected is advanced whether the user chose to
include one that hadn't been included, or to not include one that had been.

If you really feel the need for some element in the UI to "include and
advance", IMHO it makes more sense to have a specific button for that
purpose, and to let the checkbox just be a checkbox.

I tried to post all the necessary code, and I believe you found the bug. Now
I need to use a different event for the radiobutttons other than
CheckedChanged. The first one that comes to mind is Click. If you have any
other suggestions let me know.

If you really insist that the current UI is what you want, then IMHO the
appropriate solution may not be centered on the UI at all.

If you've heard of the MVC design pattern, this will be familiar to you.
The basic idea is that the reason for advancement isn't really that
the _checkbox_ itself changed, but rather than the image being viewed
had its "include" state changed. In that view, your checkbox is really
only a proxy for that state, and isn't the best source of information
for responding to changes to the state.

So, a better model for the situation would be for your PictureFile class
to have an event that is raised when the value of the Include property
is changed, and then have your form class subscribe to that property and
advance the current picture when it does.

It's not really MVC, but it's uses the basic concepts underlying MVC.

Here's a different implementation of your code, incorporating the
various suggestions so far, including the above (with the usual caveats
that the code hasn't been compiled, never mind tested):

namespace slideshow
{
public partial class FormMain : Form
{
List<PictureFile> pictures;
int curFileIndex;

private void GoToPicture(int index)
{
if (pictures.Count > 0)
{
pictures[curFileIndex].IncludeChanged -= IncludeChangedHandler;

curFileIndex = (index + pictures.Count) % pictures.Count;

pictures[curFileIndex].IncludeChanged += IncludeChangedHandler;

DisplayPicture();
}
}

private void IncludeChangedHandler(object sender, EventArgs e)
{
GoToNextPicture();
}

private void GoToNextPicture()
{
GoToPicture(curFileIndex + 1);
}

private void GoToPreviousPicture()
{
GoToPicture(curFileIndex - 1);
}

private void GoToFirstPicture()
{
GoToPicture(0);
}

private void DisplayPicture()
{
if (pictures.Count > 0)
{
PictureFile curFile = pictures[curFileIndex];

pboxDisplay.Image = curFile.Image;
rbInclude.Checked = curFile.Include;
rbInclude.Enabled = true;
rbDontInclude.Checked = !curFile.Include;
rbDontInclude.Enabled = true;
}
else
{
pboxDisplay.Image = null;
rbInclude.Checked = false;
rbInclude.Enabled = false;
rbDontInclude.Checked = false;
rbDontInclude.Enabled = false;
}
if (timer.Enabled)
{
timer.Stop();
timer.Start();
}
}

private void rbInclude_CheckedChanged(object sender, EventArgs e)
{
pictures[curFileIndex].Include = rbInclude.Checked;
}
}
}

//PictureFile.cs

using System.IO;
using System.Drawing;

namespace slideshow
{
class PictureFile
{
FileInfo file;
bool include = false;
Image image;

public PictureFile(string filename, Image image)
{
file = new FileInfo(filename);
this.image = image;
}

public event EventHandler IncludeChanged = delegate { };

public bool Include
{
get
{
return include;
}
set
{
if (include != value)
{
include = value;
IncludeChanged(this, EventArgs.Empty);
}
}
}

public FileInfo File
{
get
{
return file;
}
set
{
file = value;
}
}

public Image Image
{
get
{
return image;
}
set
{
image = value;
}
}
}
}

You'll note that for this particular change, the new event and handling
it complicates the code somewhat. But IMHO the overall abstraction is a
better fit for what's actually going on, and will improve
maintainability of the code in the long run.

Pete

 

[kndg]

While Peter has shown a good example using event-driven approach, I just
would like to add my current design principle when dealing with the UI.
I prefer having the code not related to the UI to be encapsulated on
another class (something like "manager" class), so when you need to
extend the functionality, you don't have to mess up the UI code. For
example (not properly tested),

public class PictureManager
{
private readonly List<PictureFile> pictures = new List<PictureFile>();
private int curFileIndex = 0;

public PictureFile CurrentPicture
{
get
{
return (pictures.Count < 1) ? null : pictures[curFileIndex];
}
}

public void Add(PictureFile pictureFile)
{
pictures.Add(pictureFile);
}

private void GoToPicture(int index)
{
if (pictures.Count > 0)
{
curFileIndex = (index + pictures.Count) % pictures.Count;
}
}

public void AdvancePicture(bool skipUnselected)
{
int i = 0;

do
{
GoToPicture(curFileIndex + 1);
}
while (skipUnselected && ++i < pictures.Count &&
!pictures[curFileIndex].IsSelected);
}

public void PreviousPicture(bool skipUnselected)
{
int i = 0;

do
{
GoToPicture(curFileIndex - 1);
}
while (skipUnselected && ++i < pictures.Count &&
!pictures[curFileIndex].IsSelected);
}

public void GoToFirstPicture()
{
GoToPicture(0);
}

public void Export()
{
foreach (var picture in pictures)
{
if (picture.IsSelected)
{
// do something
}
}
}
}

public class PictureFile
{
public FileInfo File { get; private set; }
public Image Image { get; private set; }
public bool IsSelected { get; set; }

public PictureFile(string filename)
{
File = new FileInfo(filename);
Image = Image.FromFile(filename);
IsSelected = false;
}
}

So, in your main form, when the user click the advance button, you just
call pictureManager.Advance(false) (or pictureManager.Advance(true) if
you like to skip not selected image) and call
pictureManager.CurrentPicture.Image to get the image.

Regards.

 

[kndg]

I should have note that since you are using unmanaged resource (Image),
both PictureFile and PictureManager class should implement the
IDisposable interface.

 

[kndg]

I play around and create the UI and notice the heavy delay in image
loading. So, I modify the PictureFile class to return the image only
when needed. Here is the modified PictureFile class.

public class PictureFile : IDisposable
{
public FileInfo File { get; private set; }
public bool IsSelected { get; set; }

private Image image;

public Image Image
{
get
{
if (image == null)
{
image = Image.FromFile(File.FullName);
}

return image;
}
}

public PictureFile(string filename)
{
File = new FileInfo(filename);
IsSelected = false;
}

public void Dispose()
{
if (image != null)
{
image.Dispose();
}
}
}

Regards.

 

[Peter Duniho]

While Peter has shown a good example using event-driven approach, I just
would like to add my current design principle when dealing with the UI.
I prefer having the code not related to the UI to be encapsulated on
another class (something like "manager" class), so when you need to
extend the functionality, you don't have to mess up the UI code.

In the MVC pattern, that class could be the "controller" of the
"model-view-controller" trilogy. Fully implementing MVC would
definitely be an improvement, but it is possible to get much of the
benefit without the additional overhead, especially in simpler programs.

[...]
So, in your main form, when the user click the advance button, you just
call pictureManager.Advance(false) (or pictureManager.Advance(true) if
you like to skip not selected image) and call
pictureManager.CurrentPicture.Image to get the image.

I'll leave it to Wannabe to evaluate whether your code example addresses
his needs or not. I will point out a couple of things:

– It is generally a bad idea to have a single method that does two
different things based on a flag passed to it. This is especially true
for the public API of the class, but IMHO it makes sense to try to
follow that for private members too.

In fact, I recently ran across an article that articulates this view in
a reasonably entertaining and yet informative way:
http://www.informit.com/articles/article.aspx?p=1392524

– One thing I do note about the looping methods you posted is that
they could easily be refactored into a single helper method. The public
API can be cleaned up, and the private implementation consolidated, as
follows:

class PictureManager
{
// Other private members and methods as before

public void NextPicture()
{
GoToPicture(1);
}

public void NextIncludedPicture()
{
GoToIncludedPicture(1);
}

public void PreviousPicture()
{
GoToPicture(-1);
}

public void PreviousIncludedPicture()
{
GoToIncludedPicture(-1);
}

private void GoToPicture(int increment)
{
if (pictures.Count > 0)
{
curFileIndex = (curFileIndex + increment + pictures.Count) %
pictures.Count;
}
}

private void GoToIncludedPicture(int increment)
{
for (int i = 0; i < pictures.Count; i++)
{
GoToPicture(increment);
if (pictures[curFileIndex].IsSelected)
{
break;
}
}
}
}

I also note that the above "manager" class would also need some tie-in
with the actual UI class. This could either be explicit calls to the
UI, or perhaps via an event-drived API by which the UI code can update
itself based on observable changes that occur in the manager class. I'd
prefer the latter, but either should work fine.

Note that you might actually consider the "manager" to be a model,
rather than a controller. In that event, you'd still have a separate
controller class that ties everything together, by subscribing to
changes in the "manager" model and updating the UI as appropriate.

There are lots of different possibilities, with the relative importance
of the more elaborate abstractions increasing as the overall complexity
of the program also increases. For a relatively simple program, all
that might be overkill.

Notes on other stylistic changes I made:

– Changing "Advance" back to "Next": note that "Advance" is a verb,
while "Previous" is an adjective. IMHO, it makes more sense for the
method naming to be consistent. So, either "Next" and "Previous" (i.e.
two adjectives), or "Advance" and "Decrease" (or maybe "Retreat",
"Regress", "Reverse", etc. – the key being, two verbs).

– Switching from a "do/while" to a "for" loop: note that the do/while
always performs the action. But if the count is 0, that's pointless.
Also, IMHO a for loop expresses the usage of the index variable better,
because it's a readily recognized pattern to practically any programmer,
whereas a do/while that includes a pre-incremented index variable takes
a little more thinking.

Have we beaten Wannabe's poor code to death yet?

Pete

 

[kndg]

– It is generally a bad idea to have a single method that does two
different things based on a flag passed to it. This is especially true
for the public API of the class, but IMHO it makes sense to try to
follow that for private members too.

In fact, I recently ran across an article that articulates this view in
a reasonably entertaining and yet informative way:
http://www.informit.com/articles/article.aspx?p=1392524

Hmm...interesting article.
The reason I use bool flag is to make it easy for the UI code to pass
the argument. For example (assume cbxSelected is a CheckBox that set the
picture for exporting),

Instead of this,

if (cbxSelected.Check)
{
// advance picture to next selected image
}
else
{
// advance picture normally
}

I would just use,

pictureManager.NextPicture(cbxSelected.Checked);

But surely, having extra methods to trade in for code clarity and future
maintenance is indeed a very wise investment.

– One thing I do note about the looping methods you posted is that they
could easily be refactored into a single helper method. The public API
can be cleaned up, and the private implementation consolidated, as follows:

class PictureManager
{
// Other private members and methods as before

public void NextPicture()
{
GoToPicture(1);
}

public void NextIncludedPicture()
{
GoToIncludedPicture(1);
}

public void PreviousPicture()
{
GoToPicture(-1);
}

public void PreviousIncludedPicture()
{
GoToIncludedPicture(-1);
}

private void GoToPicture(int increment)
{
if (pictures.Count > 0)
{
curFileIndex = (curFileIndex + increment + pictures.Count) %
pictures.Count;
}
}

private void GoToIncludedPicture(int increment)
{
for (int i = 0; i < pictures.Count; i++)
{
GoToPicture(increment);
if (pictures[curFileIndex].IsSelected)
{
break;
}
}
}
}

Nice! You are a master in the art of refactoring.
But I'm having a problem with the word "Included". Though I'm not a
native english speaker, instead of "Included", I think the word
"Selected" is more natural to me (NextSelectedPicture(),
PreviousSelectedPicture() and GoToSelectedPicture()). Am I right?

[...]

Notes on other stylistic changes I made:

– Changing "Advance" back to "Next": note that "Advance" is a verb,
while "Previous" is an adjective. IMHO, it makes more sense for the
method naming to be consistent. So, either "Next" and "Previous" (i.e.
two adjectives), or "Advance" and "Decrease" (or maybe "Retreat",
"Regress", "Reverse", etc. – the key being, two verbs).

Hahaha... I wondered why I changed that. Yes, you're correct.

[...]
Have we beaten Wannabe's poor code to death yet?

Hmm, I don't expect that. My hope is for whatever code post to this NG
could be of some guidance to others and also open for opportunity to
others making an improvement.

Regards.

 

[Peter Duniho]

The reason I use bool flag is to make it easy for the UI code to pass
the argument.[...]

But surely, having extra methods to trade in for code clarity and future
maintenance is indeed a very wise investment.

My thoughts too. Note also that your motivation to be able to pass the
flag directly from the UI, while well-intentioned, is a subtle
dependency on the UI implementation for the class that's supposedly
written specifically to provide that separation of UI and non-UI logic.

The thing I like most about the separate methods is that it gives you a
chance to name the methods something more explanatory and specific to
their purpose, allowing the code to be more self-documenting.

[...]
But I'm having a problem with the word "Included". Though I'm not a
native english speaker, instead of "Included", I think the word
"Selected" is more natural to me (NextSelectedPicture(),
PreviousSelectedPicture() and GoToSelectedPicture()). Am I right?

I just borrowed the word "Included" from the OP's original code. I gave
it practically no thought at all.

That said, in his context I think "Included" might make more sense. To
me, "Selected" implies a selection, which is a transient condition.
I.e. GUI programs often have content that may be selected as the user
operates the program, but that selection is expected to change on a
regular basis.

On the other hand, the OP is apparently implementing a slide show type
program, where presumably one might indicate in the program which images
are to be "included" in the slide show and which are not. Inasmuch as
the act of making that indication could still be considered "selecting"
the images, the word "Selected" isn't necessarily _wrong_ per se. But
in the broader context, "Included" might be more consistent with the
overall paradigm the program is using.

[...]
Have we beaten Wannabe's poor code to death yet?

Hmm, I don't expect that. My hope is for whatever code post to this NG
could be of some guidance to others and also open for opportunity to
others making an improvement.

Well, thank goodness it's not an actual horse then. Could get messy
otherwise!

Pete

 

[Andy O'Neill]

kndg wrote:

Well, thank goodness it's not an actual horse then. Could get messy
otherwise!

Pete

I don't know whether wannabe is standing bespattered and bemused by a
carcass or eagerly consigned it to the knackers yard already.

Myself, I like the approach and code.
I'm impressed at the amount of time spent answering people's questions.

 

[wannabe geek]

No, don't do that. Remove "curFile" as "kndg" suggested.

It is simpler to use curFile than curFileIndex.

I still think your problem is with your event handler.

Yes, it was. Everything is under control, the program compiled, and in use.
It works perfectly. My first try at getting a large number of PictureFiles
you can probably guess what happened. I ran out of memory. I removed the
Image property and dynamically loaded the image from a file.

This is meant to be a multipurpose program. You can view a slide show or you
can preview pictures, select some, and export them to another folder to be
printed etc. later. The second use is the reason for the advancement of the
slide show when the person decides whether they want the picture or not. They
do not need to preview it any longer and wish to see the next picture.

If you all wish to pick apart, change, put together, pick apart and destroy
my code to get something that is ideal for everybody but me, go ahead.