Bug and Patch: limitStart is ignored if it's greater than the number of results
#1
I just put in a pull request with this on github, but I'm very new to github and not sure I did that correctly so I wanted to follow up here.  Please advise me if I did something wrong, I may have another few bugfixes to contribute and want to make sure I'm going through the proper process.

Note that 1 test in the test suite ("make test") is failing for me right now: that test fails with or without my patch and appears completely unrelated.

Currently, if the query code needs to apply limits manually it ignores a limitStart which is greater than limitEnd.  So if there are only 10 movies, and I request movies 11-20 sorted by dateadded, then it returns movies 1-10 again.
  • This can obviously break paginated interfaces that use the JSON API to query for results
  • It's a bad incoherence based on sorting: requesting movies 11-20 with no sort uses the database limits, so correctly returns 0 items.  But the same request sorted by dateadded returns 10 results.
My fix is at https://github.com/xbmc/xbmc/pull/18466, and consists solely of the following diff:
Quote:diff --git a/xbmc/utils/SortUtils.cpp b/xbmc/utils/SortUtils.cpp
index 840e69ec06..b5c4b3cdfc 100644
--- a/xbmc/utils/SortUtils.cpp
+++ b/xbmc/utils/SortUtils.cpp
@@ -965,6 +965,11 @@ void SortUtils::Sort(SortBy sortBy, SortOrder sortOrder, SortAttribute attribute
     items.erase(items.begin(), items.begin() + limitStart);
     limitEnd -= limitStart;
   }
+  else if (limitStart > 0)
+  {
+    items.erase(items.begin(), items.end());
+    limitEnd = 0;
+  }
   if (limitEnd > 0 && (size_t)limitEnd < items.size())
     items.erase(items.begin() + limitEnd, items.end());
 }
@@ -1004,6 +1009,11 @@ void SortUtils::Sort(SortBy sortBy, SortOrder sortOrder, SortAttribute attribute
     items.erase(items.begin(), items.begin() + limitStart);
     limitEnd -= limitStart;
   }
+  else if (limitStart > 0)
+  {
+    items.erase(items.begin(), items.end());
+    limitEnd = 0;
+  }
   if (limitEnd > 0 && (size_t)limitEnd < items.size())
     items.erase(items.begin() + limitEnd, items.end());
 }
Reply
#2
Looks like you've done everything right regarding the PR submission. In advance i'll say thanks for the PR, always appreciated.

Your reasoning sounds sane for the change, hopefully we'll have someone have a look over it in the next day or two.
Reply

Logout Mark Read Team Forum Stats Members Help
Bug and Patch: limitStart is ignored if it's greater than the number of results0