qt: limit=0 should mean limit=0 in `BaseModel`

limit=0 having special value, such as loading all items is not intuitive. limit=0 should behave as limit is 0, as what it suggests.

Not only that but it also means we need two special values to (current case):

• Not load any items. (any negative number?)
• Load all items. (limit=0)

Where as if limit=0 behaves as limit=0, we need only one special value to load all items. I propose any arbitrary negative number.

The current situation also makes mapping more complicated, for example with a view that has a property itemCountToShow, we can not simply map limit: view.itemCountToShow, because when itemCountToShow is 0, this would cause the model to load all items.

I have previously mentioned about this peculiar behavior in the upcoming home page merge request where we suffer from this problem: !5890 (comment 475422), !5890 (comment 478733)

Request review @chub.

mentioned in merge request !5890

added MRStatus::NotCompliant label

added 1 commit

• b3b4c9e2 - qt: make limit=0 behave limit=0 in `BaseModel`

Compare with previous version

added 1 commit

• 9407f1fa - qt: make limit=0 behave limit=0 in `BaseModel`

Compare with previous version

added 1 commit

• cb7b46c0 - qt: make limit=0 behave limit=0 in `BaseModel`

Compare with previous version

added MRStatus::Reviewable label and removed MRStatus::NotCompliant label

changed milestone to %4.0

added Component::Interface: Qt label

requested review from @chub

added MRStatus::WaitingForReviewerFeedback label and removed MRStatus::Reviewable label

0 is the special value of the medialibrary to return all objects

https://code.videolan.org/videolan/medialibrary/-/blob/d431d3c111d4e75b262b5a00aa2af4e73a985f20/include/medialibrary/IQuery.h

so unless you also set an offset (If I understand correctly) this will fetch all objects

That's really unfortunate if that's the case. I thought that the change here would be enough:

            size_t queryCount = (m_limit >= 0)
                ? std::min(static_cast<size_t>(m_limit), maximumCount)
                : maximumCount;

So early return here if limit is 0, without going to media library, as well as here?

            unsigned int loaded = q->getLoadedCount();
            int count = q->getCount();
            int limit = q->getLimit();
            //item doesn't exists
            if ((limit >= 0 && count >= limit) || loaded == getMaximumCount()) {
                cb(std::nullopt);
            }

yes, 0 limit can be handled with early returns

I have added early return to in void ListCache<T>::asyncCountAndLoad(), but when I checked this, early return did not make a difference (current approach worked fine). Are we sure we need that there (I want to be cautious to not cause issues with early return by missing some important steps) ?

The other place where limit is used, getIndexFromIdImpl() seems that it already returns early.

I have not found another place where limit is used.

@chub, @fuzun

Can we apply this change in the medialibrary as well?

I just raised medialibrary!778 in medialibrary to allow search capability in history (for Continue Watching). Should I raise an MR for this change as well in the medialibrary?

Can we apply this change in the medialibrary as well?

that may be complicated to change the API, medialibrary is used by other platforms (android, ios)

Are we sure we need that there

I wrote an unit test that illustrate it.

diff --git a/modules/gui/qt/tests/test_ml_model.cpp b/modules/gui/qt/tests/test_ml_model.cpp
index 7ecc788b94..6cbe56f92a 100644
--- a/modules/gui/qt/tests/test_ml_model.cpp
+++ b/modules/gui/qt/tests/test_ml_model.cpp
@@ -77,6 +77,7 @@ protected:
         {
             VLC_UNUSED(ml);
             VLC_UNUSED(queryParams);
+            const_cast<MLTestModel*>(&m_mlTestModel)->m_countCalls += 1;
             return m_mlTestModel.m_items.size();
         }
 
@@ -84,6 +85,8 @@ protected:
         {
             VLC_UNUSED(ml);
 
+            const_cast<MLTestModel*>(&m_mlTestModel)->m_loadCalls += 1;
+
             if (m_mlTestModel.m_items.empty())
                 return {};
 
@@ -108,6 +111,18 @@ protected:
         const MLTestModel& m_mlTestModel;
     };
 
+public:
+    int getLoadCalls() const {
+        return m_loadCalls;
+    }
+
+    int getCountCalls() const {
+        return m_countCalls;
+    }
+
+private:
+    std::atomic<int> m_loadCalls = 0;
+    std::atomic<int> m_countCalls = 0;
 
     std::vector<MLItemId> m_items;
 };
@@ -208,6 +223,17 @@ private slots:
         QCOMPARE(row, mid - 1);
     }
 
+    void testLimit0PreformsNoLoad()
+    {
+        m_model->setLimit(0);
+        QCOMPARE(m_model->getLoadCalls(), 0);
+        m_model->resetRequested();
+        const int timeout = 1000;
+        // let loading complete
+        QVERIFY(QTest::qWaitFor([this] () { return !m_model->loading(); }, timeout));
+        QCOMPARE(m_model->getLoadCalls(), 0);
+    }
+
 private:
     std::unique_ptr<VLCTestingEnv> m_env;
     std::unique_ptr<MediaLib> m_medialib;

I wrote an unit test that illustrate it.

Do you mean this?

            size_t queryCount = (m_limit >= 0)
                ? std::min(static_cast<size_t>(m_limit), maximumCount)
                : maximumCount;

            if (queryCount == 0) return;

Past that CacheData is constructed with 0 queryCount.

What if m_needReload is true? That would miss the partialUpdate() call.

@chub, @fuzun
Hello mentors!

Since medialibrary uses sql, I think it should have followed the behaviour of LIMIT 0 of sql which is to fetch no fields from a relation. To fetch all the fields from a relation medialibrary should have used a large constant as limit when the limit provided in the api call was negative.

Below is a reference from the mysql docs. Though medialibrary uses sqlite, I couldn't find an official sqlite doc on LIMIT.

To retrieve all rows from a certain offset up to the end of the result set, you can use some large number for the second parameter. This statement retrieves all rows from the 96th row to the last:

SELECT * FROM tbl LIMIT 95,18446744073709551615;

In this MR we are basically trying to do 2 tasks:

1. "Ignore" and "un-use" this OFFSET 0 LIMIT 0 meaning fetching all fields behaviour of medialibrary. We need to STOP ourselves from making the api call to medialibrary when the offset and limit are both 0. It's okay to make the call when the offset is not 0, but the limit is 0.
2. Emulate the new behavior of passing the limit < 0, meaning limit = A LARGE CONSTANT. (as referenced above) So that limit < 0 can fetch all the fields from the relation.

Now:

1. To STOP ourselves from making api call to medialibrary on offset = 0 and limit 0, we need to have a condition at the place where we are actually calling the count and load medialibrary api related functions (eg. MLVideoModel::Loader::count, MLVideoModel::Loader::load, etc.) that we set in each ml model.
   
   Those places seems to be MLListCacheLoader::countTask, MLListCacheLoader::loadTask, and MLListCacheLoader::countAndLoadTask, where medialibrary apis are actually being called.

2. To emulate the behaviour of limit < 0 means limit = A LARGE CONSTANT, we need to manually set the limit to a large and safe constant before passing it to the medialibrary api, when the limit provided is negative, at that place where the query parameters are being set.
   
   That place seems to the MLListCacheLoader::MLOp::getQueryParams.

Additionally, I believe there's really no requirement to have early returns in listcache.hxx. That file seems to be dealing with all the caching part after the medias are fetched from the medialibary.

I have made the above mentioned changes in my this remote branch after cloning the branch of this MR and rebasing the same commit. I'd like to request you to have a look at it.

Since medialibrary uses sql, I think it should have followed the behaviour of LIMIT 0 of sql

I agree. Past is past, but maybe this can be changed directly in medialibrary, especially if there is a major version bump soon.

I don't know SQL much, but why use LIMIT at all if limit is not wanted. To disable limit, there could be a flag or repurposing the sign of the limit (as I proposed, negative limit disables the limit. This comes with a disadvantage of limited maximum value, but I thought this is okay for now because Qt's models use int anyway) can be used.

We need to STOP ourselves from making the api call to medialibrary when the offset and limit are both 0

I don't want to care about the offset in this merge request, is it necessary? I only want to make limit 0 behave like limit 0, and I assumed that with

            size_t queryCount = (m_limit >= 0)
                ? std::min(static_cast<size_t>(m_limit), maximumCount)
                : maximumCount;

If the limit is below 0, the maximum count would be used. This seemed to be the current behavior to me when limit is 0.

Emulate the new behavior of passing the limit < 0, meaning limit = A LARGE CONSTANT

My intention was to disable the limit, if possible. I'm not sure if it is possible, but a large constant or maximum count would also be fine for now. Isn't that currently what is done when limit is 0? We don't need to care too much about whether negative limit disables the limit, as long as it loads all items.

If limit negative does not work as expected here, I can use maximum value of ssize_t as its default value. Alternatively if limit is negative, it can be made 0 to load all items (current behavior when limit is 0).

I have made the above mentioned changes in my this remote branch

@chub should comment on it. I'm not sure if additions are necessary (I only want bare necessities, I don't intend to achieve ideality here). If these are required, why not call all() instead of items() if limit is negative instead of dealing with offset?

I'm willing to let others work on this if they want to do this more idiomatically.

why use LIMIT at all if limit is not wanted

My intention was to disable the limit

As Pierre mentioned, changing the OFFSET 0 LIMIT 0 medialibrary behavior will require changing it in all projects where medialibrary is used. I believe we should really not do any change in the medialibrary for this MR. Not setting the limit in the sql query when limit < 0 is perfect, but that is a medialibrary level change.

I don't want to care about the offset in this merge request, is it necessary? I only want to make limit 0 behave like limit 0

I mentioned that we must NOT call the medialibrary api functions when offset is 0 and limit is 0. Yes, we can also not call the api functions when only limit is 0. (But is that ideal? In the case when offset is non-zero and limit is 0 medialibrary is anyway going to fetch 0 medias --- the problem is only when offset and limit are both 0, so we fix this by not making calls to medialibrary in this case. I believe we should not generalise this fix for all cases when limit is 0 --- by not making a call to medialibrary when limit is 0.)

Isn't that currently what is done when limit is 0?

I guess you are asking how the current (faulty) behaviour of OFFSET 0 LIMIT 0 is implemented. It's not by setting a large value, but the logic is written to return all medias and return without interfering with the sql query. (see 1, 2)

We don't need to care too much about whether negative limit disables the limit, as long as it loads all items.

I think I didn't get this one, sorry.

I can use maximum value of ssize_t as its default value

That's really dangerous, because in the actual C medialibrary apis, limit (known as i_nbResults) has the type uint32_t. (see) That's the reason I mentioned to decide a large and safe big limit to pass when limit < 0. I think XYZ_MAX should not be used, but instead a hardcoded large constant, like 1e7/1e8/1e9.

Alternatively if limit is negative, it can be made 0 to load all items (current behavior when limit is 0).

This won't work in the case when you have a non-zero offset. Limit 0 fetches all medias ONLY when the offset is also zero. (see 1, 2)

I'm not sure if additions are necessary

There weren't the changes to not call the medialibrary api related functions when offset and limit are both 0, I've added them in my branch. It is necessary along with the changes in the listcache.hxx which deals with caching after the data is fetched from the medialibrary.

I believe we should really not do any change in the medialibrary for this MR. Not setting the limit in the sql query when limit < 0 is perfect, but that is a medialibrary level change.

I do not really care about the medialibrary in this merge request, I wanted to make a generic comment. I don't plan to open a merge request there either. But my personal opinion is it would be better if medialibrary changes this behavior as soon as feasible. Since it uses unsigned numbers, it probably needs an extra boolean variable to control whether to have limit or not, rather than repurposing the sign bit of the limit as a limit disabler as I tried to do here.

I mentioned that we must NOT call the medialibrary api functions when offset is 0 and limit is 0. Yes, we can also not call the api functions when only limit is 0.

My understanding is that we should never pass through our limit to medialibrary, because starting from this merge request our interpretation and medialibrary's intention starts to differ. Qt interface treats limit 0 as what the name suggests, while medialibrary keeps loading all items when limit is 0.

I don't know where the offset comes into play, and it is not clear to me why we need to care about the offset.

I guess you are asking how the current (faulty) behaviour of OFFSET 0 LIMIT 0 is implemented. It's not by setting a large value, but the logic is written to return all medias and return without interfering with the sql query. (see 1, 2)

That is the ideal behavior with disabled limit, such as when it has negative value. Since medialibrary works with unsigned numbers, negative is meaningless for it. It seems that I need to make limit "0" before passing it to medialibrary if it is negative.

I think I didn't get this one, sorry.

I wanted to mean that as long as limit negative makes the model load all items, we don't need to necessarily care if LIMIT is used when querying or not.

That's really dangerous, because in the actual C medialibrary apis, limit (known as i_nbResults) has the type uint32_t.

But we already have that problem, as Qt's models use bare integer int type.

If medialibrary does not allow us to disable limiting, besides using 0, then it is medialibrary's problem and we can use maximum number. I guess passing the limit as 0 workaround I mentioned above makes more sense than this workaround, though.

This won't work in the case when you have a non-zero offset

That is medialibrary decision. It looks like an already existing case. What happens currently when limit is 0, but offset is not? The same would happen when limit is negative (whether we pass it as 0 if it is negative to media library), but offset is not.

I make changes with regard to the limit here, I'm not sure if offset should start to be treated differently.

It seems that I need to make limit "0" before passing it to medialibrary if it is negative.

I checked this again now. I could not find more places where the limit is used. I found only MLBaseModelPrivate::getIndexFromIdImpl() and ListCache<T>::asyncCountAndLoad(). I already changed these here. This was what I wanted to mean by "I have not found another place where limit is used.". It is possible I missed some, please do let me know.

And from the looks of this with my limited understanding of medialibrary, we already do not pass limit 0 to medialibrary, even if the limit is 0 (where it seems we should in current situation, why not is this the case @chub ?):

            size_t queryCount = (m_limit > 0)
                ? std::min(m_limit, maximumCount)
                : maximumCount;

If currently limit=0 still makes medialibrary to use limit ("maximumCount", so I assume all items are loaded), I have zero intention to fix this in this merge request (I would rather continue doing the same when limit is negative). My aim is to not cause regressions with workarounds, not fix existing bugs especially in fields I have no experience. This is a minefield for me.

PASS   : TestVLCMLModel::initTestCase()
load    0 100
count    0 100
load    0 100
count    0 100
PASS   : TestVLCMLModel::testModelRecuseUpdateFromSlot()
load    0 100
count    0 100
load    0 100
count    0 100
load    101 200
PASS   : TestVLCMLModel::testGetIndexFromID()
load    0 100
count    0 100
load    0 100
count    0 100
load    0 100
count    0 100
FAIL!  : TestVLCMLModel::testLimit0PreformsNoLoad() Compared values are not the same
   Actual   (m_model->getLoadCalls()): 2
   Expected (0)                      : 0

Pierre's provided test is failing on the current changes of this MR.

(I also added std::cout << "count " << queryParams->i_offset << " " << queryParams->i_nbResults << std::endl and std::cout << "load " << queryParams->i_offset << " " << queryParams->i_nbResults << std::endl in the respective functions to get more logs.)

This is because the data is loaded in chunks here, and count is passed as the limit to medialibrary here.

Pierre's provided test is failing on the current changes of this MR.

I asked @chub here !7047 (comment 480990) what are the consequences of early return to prevent passing 0 to media library. As I said, I can early return, but I want to make sure it does not cause regressions.

This happens because starting with this merge request,

Qt interface treats limit 0 as what the name suggests, while medialibrary keeps loading all items when limit is 0.

what are the consequences of early return to prevent passing 0 to media library. As I said, I can early return

We should have some kind of early returns here, but we need to ensure that pre-existing cache will be cleared properly to avoid retaining outdated data.

We should have some kind of early returns here, but we need to ensure that pre-existing cache will be cleared properly to avoid retaining outdated data.

I'm not sure if I'm able to do that. If this is not easy to achieve, then I think that @tvermaashutosh can map 0 to 1 for what it's worth. I don't want to block the home page merge request.

I think that @tvermaashutosh can map 0 to 1

@fuzun
Yeah!
I think we should not consider this MR as a dependency MR for !5890 and have 1 as the fallback limit there. When this MR gets ready, it can modify the HomePage.qml as well.
Additionally, in !5890 we also need to decide on how to use the header prop of Page.

Additionally, in !5890 we also need to decide on how to use the header prop of Page.

Yeah, that is also not easy to fix. I guess we either need to move the page header back to content item with a fixme note, or remove the page header if we don't want to block the home page more.

Flickable can be centralized instead of having it in the page's content item, such as being a parent of stackView in MainDisplay.qml, but then we can end up with nested flickable if the page is/has an item view (that derives from Flickable) which is technically all pages except the home and browse page.

It needs to be investigated if the parent Flickable can be neutralized when the page has a main (child) flickable, otherwise it can end up with a lot of problems.

added MRStatus::Reviewable label and removed MRStatus::WaitingForReviewerFeedback label

added MRStatus::InReview label and removed MRStatus::Reviewable label

nit tab