Merge branch 'master' of git://github.com/gallery/gallery3

4 files changed, 114 insertions, 80 deletions

diff --git a/modules/gallery/controllers/albums.php b/modules/gallery/controllers/albums.php
index 08a60132..3ea08538 100644
--- a/modules/gallery/controllers/albums.php
+++ b/modules/gallery/controllers/albums.php
@@ -39,7 +39,8 @@ class Albums_Controller extends Items_Controller {
     $show = $this->input->get("show");
 
     if ($show) {
-      $index = $album->get_position($show);
+      $child = ORM::factory("item", $show);
+      $index = $album->get_position($child);
       if ($index) {
         $page = ceil($index / $page_size);
         if ($page == 1) {
diff --git a/modules/gallery/controllers/movies.php b/modules/gallery/controllers/movies.php
index 04e15315..fa07668e 100644
--- a/modules/gallery/controllers/movies.php
+++ b/modules/gallery/controllers/movies.php
@@ -22,42 +22,32 @@ class Movies_Controller extends Items_Controller {
   /**
    *  @see REST_Controller::_show($resource)
    */
-  public function _show($photo) {
-    access::required("view", $photo);
+  public function _show($movie) {
+    access::required("view", $movie);
 
-    // We sort by id ascending so for now, find sibling info by doing id based queries.
-    $next_item = ORM::factory("item")
-      ->viewable()
-      ->where("parent_id", $photo->parent_id)
-      ->where("id >", $photo->id)
-      ->orderby("id", "ASC")
-      ->find();
-    $previous_item = ORM::factory("item")
-      ->viewable()
-      ->where("parent_id", $photo->parent_id)
-      ->where("id <", $photo->id)
-      ->orderby("id", "DESC")
-      ->find();
-    $position = ORM::factory("item")
-      ->viewable()
-      ->where("parent_id", $photo->parent_id)
-      ->where("id <=", $photo->id)
-      ->count_all();
+    $position = $movie->parent()->get_position($movie);
+    if ($position > 1) {
+      list ($previous_item, $ignore, $next_item) =
+        $movie->parent()->children(3, $position - 2);
+    } else {
+      $previous_item = null;
+      list ($next_item) = $movie->parent()->viewable()->children(1, $position);
+    }
 
     $template = new Theme_View("page.html", "movie");
-    $template->set_global("item", $photo);
+    $template->set_global("item", $movie);
     $template->set_global("children", array());
-    $template->set_global("children_count", $photo->children_count());
-    $template->set_global("parents", $photo->parents());
-    $template->set_global("next_item", $next_item->loaded ? $next_item : null);
-    $template->set_global("previous_item", $previous_item->loaded ? $previous_item : null);
-    $template->set_global("sibling_count", $photo->parent()->children_count());
+    $template->set_global("children_count", 0);
+    $template->set_global("parents", $movie->parents());
+    $template->set_global("next_item", $next_item);
+    $template->set_global("previous_item", $previous_item);
+    $template->set_global("sibling_count", $movie->parent()->viewable()->children_count());
     $template->set_global("position", $position);
 
     $template->content = new View("movie.html");
 
-    $photo->view_count++;
-    $photo->save();
+    $movie->view_count++;
+    $movie->save();
 
     print $template;
   }
@@ -65,21 +55,21 @@ class Movies_Controller extends Items_Controller {
   /**
    * @see REST_Controller::_update($resource)
    */
-  public function _update($photo) {
+  public function _update($movie) {
     access::verify_csrf();
-    access::required("view", $photo);
-    access::required("edit", $photo);
+    access::required("view", $movie);
+    access::required("edit", $movie);
 
-    $form = photo::get_edit_form($photo);
+    $form = photo::get_edit_form($movie);
     if ($valid = $form->validate()) {
-      if ($form->edit_item->filename->value != $photo->name ||
-          $form->edit_item->slug->value != $photo->slug) {
+      if ($form->edit_item->filename->value != $movie->name ||
+          $form->edit_item->slug->value != $movie->slug) {
         // Make sure that there's not a name or slug conflict
         if ($row = Database::instance()
             ->select(array("name", "slug"))
             ->from("items")
-            ->where("parent_id", $photo->parent_id)
-            ->where("id <>", $photo->id)
+            ->where("parent_id", $movie->parent_id)
+            ->where("id <>", $movie->id)
             ->open_paren()
             ->where("name", $form->edit_item->filename->value)
             ->orwhere("slug", $form->edit_item->slug->value)
@@ -98,16 +88,16 @@ class Movies_Controller extends Items_Controller {
     }
 
     if ($valid) {
-      $photo->title = $form->edit_item->title->value;
-      $photo->description = $form->edit_item->description->value;
-      $photo->slug = $form->edit_item->slug->value;
-      $photo->rename($form->edit_item->filename->value);
-      $photo->save();
-      module::event("item_edit_form_completed", $photo, $form);
+      $movie->title = $form->edit_item->title->value;
+      $movie->description = $form->edit_item->description->value;
+      $movie->slug = $form->edit_item->slug->value;
+      $movie->rename($form->edit_item->filename->value);
+      $movie->save();
+      module::event("item_edit_form_completed", $movie, $form);
 
-      log::success("content", "Updated movie", "<a href=\"{$photo->url()}\">view</a>");
+      log::success("content", "Updated movie", "<a href=\"{$movie->url()}\">view</a>");
       message::success(
-        t("Saved movie %movie_title", array("movie_title" => $photo->title)));
+        t("Saved movie %movie_title", array("movie_title" => $movie->title)));
 
       print json_encode(
         array("result" => "success"));
@@ -121,9 +111,9 @@ class Movies_Controller extends Items_Controller {
   /**
    *  @see REST_Controller::_form_edit($resource)
    */
-  public function _form_edit($photo) {
-    access::required("view", $photo);
-    access::required("edit", $photo);
-    print photo::get_edit_form($photo);
+  public function _form_edit($movie) {
+    access::required("view", $movie);
+    access::required("edit", $movie);
+    print photo::get_edit_form($movie);
   }
 }
diff --git a/modules/gallery/controllers/photos.php b/modules/gallery/controllers/photos.php
index 79ad674a..81e7519e 100644
--- a/modules/gallery/controllers/photos.php
+++ b/modules/gallery/controllers/photos.php
@@ -25,23 +25,23 @@ class Photos_Controller extends Items_Controller {
   public function _show($photo) {
     access::required("view", $photo);
 
-    $position = $photo->parent()->get_position($photo->id);
+    $position = $photo->parent()->get_position($photo);
     if ($position > 1) {
       list ($previous_item, $ignore, $next_item) =
         $photo->parent()->children(3, $position - 2);
     } else {
       $previous_item = null;
-      list ($next_item) = $photo->parent()->children(1, $position);
+      list ($next_item) = $photo->parent()->viewable()->children(1, $position);
     }
 
     $template = new Theme_View("page.html", "photo");
     $template->set_global("item", $photo);
     $template->set_global("children", array());
-    $template->set_global("children_count", $photo->children_count());
+    $template->set_global("children_count", 0);
     $template->set_global("parents", $photo->parents());
     $template->set_global("next_item", $next_item);
     $template->set_global("previous_item", $previous_item);
-    $template->set_global("sibling_count", $photo->parent()->children_count());
+    $template->set_global("sibling_count", $photo->parent()->viewable()->children_count());
     $template->set_global("position", $position);
 
     $template->content = new View("photo.html");
diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index a87997c6..ff02daf8 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -412,39 +412,74 @@ class Item_Model extends ORM_MPTT {
    * Find the position of the given child id in this album.  The resulting value is 1-indexed, so
    * the first child in the album is at position 1.
    */
-  public function get_position($child_id) {
+  public function get_position($child) {
     if ($this->sort_order == "DESC") {
       $comp = ">";
     } else {
       $comp = "<";
     }
-
     $db = Database::instance();
-    $position = $db->query("
-      SELECT COUNT(*) AS position FROM {items}
-      WHERE `parent_id` = {$this->id}
-        AND `{$this->sort_column}` $comp (SELECT `{$this->sort_column}`
-                                          FROM {items} WHERE `id` = $child_id)")
-      ->current()->position;
-
-    // We stopped short of our target value in the sort (notice that we're using a < comparator
-    // above) because it's possible that we have duplicate values in the sort column.  An
-    // equality check would just arbitrarily pick one of those multiple possible equivalent
-    // columns, which would mean that if you choose a sort order that has duplicates, it'd pick
-    // any one of them as the child's "position".
-    //
-    // Fix this by doing a 2nd query where we iterate over the equivalent columns and add them to
-    // our base value.
-    $result = $db->query("
-      SELECT id FROM {items}
-      WHERE `parent_id` = {$this->id}
-        AND `{$this->sort_column}` = (SELECT `{$this->sort_column}`
-                                      FROM {items} WHERE `id` = $child_id)
-      ORDER BY `id` ASC");
-    foreach ($result as $row) {
-      $position++;
-      if ($row->id == $child_id) {
-        break;
+
+    // If the comparison column has NULLs in it, we can't use comparators on it and will have to
+    // deal with it the hard way.
+    $count = $db->from("items")
+      ->where("parent_id", $this->id)
+      ->where($this->sort_column, NULL)
+      ->count_records();
+
+    if (empty($count)) {
+      // There are no NULLs in the sort column, so we can just use it directly.
+      $position = $db->query("
+        SELECT COUNT(*) AS position FROM {items}
+        WHERE `parent_id` = {$this->id}
+          AND `{$this->sort_column}` $comp (SELECT `{$this->sort_column}`
+                                            FROM {items} WHERE `id` = $child->id)")
+        ->current()->position;
+
+      // We stopped short of our target value in the sort (notice that we're using a < comparator
+      // above) because it's possible that we have duplicate values in the sort column.  An
+      // equality check would just arbitrarily pick one of those multiple possible equivalent
+      // columns, which would mean that if you choose a sort order that has duplicates, it'd pick
+      // any one of them as the child's "position".
+      //
+      // Fix this by doing a 2nd query where we iterate over the equivalent columns and add them to
+      // our base value.
+      $result = $db->query("
+        SELECT id FROM {items}
+        WHERE `parent_id` = {$this->id}
+          AND `{$this->sort_column}` = (SELECT `{$this->sort_column}`
+                                        FROM {items} WHERE `id` = $child->id)
+        ORDER BY `id` ASC");
+      foreach ($result as $row) {
+        $position++;
+        if ($row->id == $child->id) {
+          break;
+        }
+      }
+    } else {
+      // There are NULLs in the sort column, so we can't use MySQL comparators.  Fall back to
+      // iterating over every child row to get to the current one.  This can be wildly inefficient
+      // for really large albums, but it should be a rare case that the user is sorting an album
+      // with null values in the sort column.
+      //
+      // Reproduce the children() functionality here using Database directly to avoid loading the
+      // whole ORM for each row.
+      $orderby = array($this->sort_column => $this->sort_order);
+      // Use id as a tie breaker
+      if ($this->sort_column != "id") {
+        $orderby["id"] = "ASC";
+      }
+
+      $position = 0;
+      foreach ($db->select("id")
+               ->from("items")
+               ->where("parent_id", $this->id)
+               ->orderby($orderby)
+               ->get() as $row) {
+        $position++;
+        if ($row->id == $child->id) {
+          break;
+        }
       }
     }
 
@@ -551,6 +586,10 @@ class Item_Model extends ORM_MPTT {
   function children($limit=null, $offset=0, $where=array(), $orderby=null) {
     if (empty($orderby)) {
       $orderby = array($this->sort_column => $this->sort_order);
+      // Use id as a tie breaker
+      if ($this->sort_column != "id") {
+        $orderby["id"] = "ASC";
+      }
     }
     return parent::children($limit, $offset, $where, $orderby);
   }
@@ -569,6 +608,10 @@ class Item_Model extends ORM_MPTT {
   function descendants($limit=null, $offset=0, $where=array(), $orderby=null) {
     if (empty($orderby)) {
       $orderby = array($this->sort_column => $this->sort_order);
+      // Use id as a tie breaker
+      if ($this->sort_column != "id") {
+        $orderby["id"] = "ASC";
+      }
     }
     return parent::descendants($limit, $offset, $where, $orderby);
   }