Slideshow using PHP and JavaScript

Question

I have been trying to create a slideshow using PHP and JavaScript mainly. However, for some reason there is no response after I click >> or << to move to the next or previous slide.

The PHP script is mainly used to grab images from a folder and form them into an array. Then I use JavaScript within this PHP via echo to transform the PHP-array into a JavaScript-array.

I used this website as a tutorial.

Thanks for your help!

<?php
//PHP SCRIPT: getimages.php
//Header("content-type: text/javascript");

//This function gets the file names of all images in the current directory
//and ouputs them as a JavaScript array
function returnimages($dirname='C:\Apache\htdocs\Psych211LifespanDevchpt1') {
    echo "in return images";
    $files = array();
    $curimage = 0;

    if ($handles = opendir($dirname)) {
        echo "in handles";
        while (false !== ($file = readdir($handles))) {
            echo "in while";
            $path_info = $_FILES['file']['name'];
            $extensions = pathinfo($path_info, PATHINFO_EXTENSION);
            echo $extensions;
            if ($extensions=='png')
            //if(eregi($pattern, $file)) { //if this file is a valid image
                //Output it as a JavaScript array element
                //$files->append($file);
                $files[$curimage] = $file;
                array_push($files,$file);
                //echo $files[$curimage];
                // 'Slides['.$curimage.']="'.$file .'";';
                //echo $curimage;
                $curimage++;
            //}
        }
        echo $curimage;
        echo count($files);
        closedir($handle);
    }
    echo '<html>
    <head>
    <script type="text/javascript"> 
    var Slides = [];
    Slides = '.json_encode($files).'; document.write(Slides.length);
    function CacheImage(ImageSource) { // TURNS THE STRING INTO AN IMAGE OBJECT
        var ImageObject = new Image();
        ImageObject.src = ImageSource;
        return ImageObject;
    }

    function ShowSlide(Direction) {
       if (SlideReady) {
          NextSlide = CurrentSlide + Direction;
          // THIS WILL DISABLE THE BUTTONS (IE-ONLY)
          document.SlideShow.Previous.disabled = (NextSlide == 0);
          document.SlideShow.Next.disabled = (NextSlide == 
    (Slides.length-1));    
     if ((NextSlide >= 0) && (NextSlide < Slides.length)) {
                document.images["Screen"].src = Slides[NextSlide].src;
                CurrentSlide = NextSlide++;
                Message = "Picture " + (CurrentSlide+1) + "of " + 
    Slides.length;
                self.defaultStatus = Message;
                if (Direction == 1) CacheNextSlide();
          }
          return true;
       }
    }

    function Download() {
       if (Slides[NextSlide].complete) {
          SlideReady = true;
          self.defaultStatus = Message;
       }
       else setTimeout("Download()", 100); // CHECKS DOWNLOAD STATUS EVERY 100 MS
       return true;
    }

    function CacheNextSlide() {
       if ((NextSlide < Slides.length) && (typeof Slides[NextSlide] == 
    "string"))
    { // ONLY CACHES THE IMAGES ONCE
          SlideReady = false;
          self.defaultStatus = "Downloading next picture...";
          Slides[NextSlide] = CacheImage(Slides[NextSlide]);
          Download();
       }
       return true;
    }

    function StartSlideShow() {
       CurrentSlide = -1;
       Slides[0] = CacheImage(Slides[0]);
       var SlideReady = true;
       ShowSlide(1);
    }

    </script>
    </head>
    <body onLoad="StartSlideShow()"> 
<form name="SlideShow"> 
<table> 
<tr> 
<td colspan=2><img src="Psych211LifespanDevchpt1/slide-1.png" name="Screen" width=108 height=135></td>
 </tr>
 <tr> 
<td><input type="button" name="Previous" value=" << " onClick="ShowSlide(-1)"></td> 
<td align="right"><input type="button" name="Next" value=" >> " onClick="ShowSlide(1)"></td> 
</table> 
</form> 
</body> 
</html>';

//return($files);


    }

    //echo 'var Slides=new Array();'; //Define array in JavaScript
    returnimages() //Output the array elements containing the image file names
    ?>

You have ``/``. I don't see a `function doPrevious()` or `function doNext()` anywhere in your code. — Sean, Apr 28 '15 at 02:51
looking at your linked tutorial, `onClick="doPrevious()"` should be `onClick="ShowSlide(-1)"` and `onClick="doNext()"` should be `onClick="ShowSlide(1)"` — Sean, Apr 28 '15 at 02:54
I am so sorry for the confusion, I copied and pasted the wrong part. It is supposed to be ` — Angela Nie, Apr 28 '15 at 03:04
@Sean I just updated the change with the code provided, hopefully this will clear my question a bit. Thanks! — Angela Nie, Apr 28 '15 at 03:09
have you checked your browser console to see if there are any error/syntax messages? have you checked the html source code to see what `Slides` looks like? — Sean, Apr 28 '15 at 03:16
@Sean, there is no error messages showing, there would just simply be no response after I clicked the ">>" button. The Slides is an array in javascript that contains all the .png files. I have checked its information and am pretty sure it's not empty. — Angela Nie, Apr 28 '15 at 03:31
[Avoid using `document.write`](https://www.youtube.com/watch?feature=player_detailpage&v=Y2Y0U-2qJMs#t=1082s), use `console.log` for debugging instead. Any tutorials that tell you to use `document.write` are over a decade out of date and shouldn't be trusted. You also have a lot of variables that are capitalized. It is valid code, but in JS it has become a [convention](http://javascript.crockford.com/code.html) that only constructor functions be capitalized, other variables should start with lowercase letters. — Useless Code, Apr 29 '15 at 04:18
Also, don't use strings with `setTimeout` (or `setInterval`), they expect a function as their first parameter. If you instead give it a string, the string is implicitly `eval`ed. Instead of `setTimeout("Download()", 100);` you just need to do `setTimeout(Download, 100);`. It is shorter and will run faster, as well as avoiding potential XSS bugs being introduced. — Useless Code, Apr 29 '15 at 04:39

score 0 · Accepted Answer · edited May 23 '17 at 10:24

Most of your functions use an implicit global variable name SlideReady. In StartSlideShow you use var SlideReady = true; to try and set it. Because you prefaced it with var you are setting a local variable that is also named SlideReady, not the global variable that other functions have access to, so the global SlideReady remains undefined.

So when StartSlideShow calls ShowSlide, the line

if (SlideReady) {

is interpreted as

if (false) {

because the global SlideReady is undefined, which is a falsy value. A falsy value is coerced to false inside the if statement, so the code inside of the if block never runs and the slide never changes.

Using a linter like jsHint or the even stricter jsLint will help you avoid these kinds of errors. Running all your code in strict mode would also have made this kind of error more apparent. There are a number of other implicit globals in this code (NextSlide, CurrentSlide, Message). Implicit globals can cause errors like this or even more insidious ones.

Other things that could be improved

window.self isn't commonly used. In particular the way this code uses it as a bare self without prefacing it with window could cause headaches. Many people use a variable named self to store a reference to this in situations where they need to access a this object of an outer function from inside another function using closure. Using self to refer to window.self will likely cause confusion for anyone reading code that uses it. (It did for me, it has been so long since I have seen a reference to window.self I forgot it existed!)

window.defaultStatus has been obsolete for a long time, most browsers don't even have a status bar anymore; no one will see the messages that are being posted there.

This code has a lot of variable names that are capitalized. It is valid code and will run, but in JavaScript it has become a convention that only constructor functions be capitalized. In simplest of terms, a constructor function is a function that needs to be called with the new keyword. If someone in the future (including you) works on the code, the function name being capitalized is a reminder that you need to use the new keyword before it. Linters also use this convention to automatically spot missing news for you.

Accessing form elements via document.formname.elementName and images via document.images are very outdated techniques. Using document.formName.elementName can also cause problems if a form element has the same name as a property of the form. Instead giving each button an idand accessing it via document.getElementById. In fact, you don't even need to use a form and buttons, it is easy to style a div or a span element to look like a button.

Using onclick is a dated way of attaching events, especially embedding it in the onclick in your HTML. It is less obtrusive to use addEventListener instead.

For a number of reasons many people writing modern JavaScript prefer to use function expressions to define functions instead of function declaration. Function declarations can cause some puzzling bugs, whereas a function expression stored in a variable behaves more predictably.

Global variables are bad and can often lead to all sorts of weird bugs, but often different parts of your code need access to common variables. You can allow this without creating globals by wrapping your code in an Immediately-Invoked Function Expression (IIFE) (sometimes also called a self-invoking function).

(function () {
  'use strict';
  var shared = true,
    foo = function () {
      // has access to shared
    },
    bar = function () {
      // also has access to shared
    };
// you can access shared here
}());
// you can't access shared here because it is not a global

I personally don't feel it is necessary or worth it to preload the images. Most people will have a reasonably fast connection and if the images are encoded properly they will load in progressively and will be viewable in some state much quicker than with this code that won't display the image until it is fully loaded. Loading the next image before the user requests it also wastes their bandwidth which could be important if they are on mobile.

If you think that preloading the images is important, instead of using an overly complex system that won't display the next picture until it is fully downloaded, I would just preload them all at the beginning:

Slides.forEach(function (url) {
  (new Image()).src = url;
});

This will download each image and store it in the browsers cache instead of keeping it in memory in an array like the code you posted does.

Modernizing the code

A more modern version of the JavaScript code from that tutorial might look more like this:

<img id="slide"></td>
<div class="slide-controls">
  <span class="button" id="prev-slide">&laquo;</span>
  <span class="button" id="next-slide">&raquo;</span>
</div>

<script>
  (function () {
    'use strict';
    var slides = [
        'http://www.placecage.com/300/200',
        'http://www.placecage.com/g/300/200',
        'http://www.placecage.com/c/300/200',
        'http://www.placecage.com/gif/300/200'
      ],
      currentSlide = 0,
      doc = document,
      elSlide = doc.getElementById('slide'),
      elPrev = doc.getElementById('prev-slide'),
      elNext = doc.getElementById('next-slide'),

      showSlide = function (index) {
        if (index > -1 && index < slides.length) {
          currentSlide = index;
          elPrev.classList.remove('disabled');
          elNext.classList.remove('disabled');
          if (index === 0) {
            elPrev.classList.add('disabled');
          } else if (index === slides.length - 1) {
            elNext.classList.add('disabled');
          }
          elSlide.src = slides[index];
          elSlide.title = 'Picture ' + (index + 1) + 'of ' + slides.length;
        }
      },
      changeSlide = function (step) {
          var index = currentSlide + step;
          showSlide(index);
      },
      prevSlide = changeSlide.bind(null, -1),
      nextSlide = changeSlide.bind(null, 1);

    elPrev.addEventListener('click', prevSlide, false);
    elNext.addEventListener('click', nextSlide, false);

    showSlide(0);
  }());
</script>

jsFiddle

There is no need to use the docuemnt load event (aka onLoad), the elements are defined in the markup above the JavaScript code and will be available when the JavaScript code runs.

Instead of using defaultStatus, I am putting the status message in the title attribute on the image, it should show up as a tool-tip in most browsers if the user mouses-over the image. You could also output the message to another <div> if you want it to be visible without a mouse-over.

I used Function.prototype.bind to create the event hanlders prevSlide and nextSlide.

The lines

  prevSlide = changeSlide.bind(null, -1),
  nextSlide = changeSlide.bind(null, 1);

are effectively equal to

  prevSlide = function () {
    changeSlide(-1);
  },
  nextSlide = function () {
    changeSlide(1);
  };

Since I separated the changeSlide logic from the actual display of the image and showSlide takes an index instead of how many forward or back to go, it would be easy to make a function that jumps to the first or the last slide too:

  var firstSlide = showSlide.bind(null, 0),
    lastSlide = showSlide.bind(null, slides.length - 1);

jsFiddle

Other resources

Here are a few resources related to the techniques I used.

Crockford on JavaScript - Section 8: Programming Style & Your Brain Is particularly applicable to some of the stuff I discussed, the whole series is worth watching. Also, reading his book "JavaScript: The good parts" helps explain the reasons for a lot of this stuff.
The Elements of JavaScript Style part one, part two
A good description of function declaration vs function expressions
Namespaces, Closures, Self-Invoking Functions, and much, much more...
JavaScript and HTML Script Tags
Named function expressions demystified

Slideshow using PHP and JavaScript

1 Answers1

Other things that could be improved

Modernizing the code

Other resources