nixd/Completion: fix isIncomplete flag handling

[Mr-Stoneman]

When typing a attribute like services.x only the first 30 solutions would be displayed and the isIncomplete flag was always set to false. Moving the bounds check fixed this and isincomplete can now be true.

[inclyc]

I don't understand how this fixes the issue. Also, regression tests should be added to ensure that the fix is verified.

[Mr-Stoneman]

This fixes the issue by checking the size of the vector after adding the last element. What was happening before this was that, there were only 30 Items, because the size was checked before adding a element the max size of Items was 29 and so never hit the 30 need to throw the error.

[Mr-Stoneman]

I ran the regression test locally, they all passed. I do not know how to add them here sry.

[inclyc]

This fixes the issue by checking the size of the vector after adding the last element. What was happening before this was that, there were only 30 Items, because the size was checked before adding a element the max size of Items was 29 and so never hit the 30 need to throw the error.

I see, then let's write some regression tests (like options-incomplete.md in #674), the file should be placed under nixd/tools/nixd/test. Just write markdown files and our test suite will discover it automatically.

[inclyc]

This fixes the issue by checking the size of the vector after adding the last element. What was happening before this was that, there were only 30 Items, because the size was checked before adding a element the max size of Items was 29 and so never hit the 30 need to throw the error.

The code you modified appears to emplace items regardless of size. If the addItem method is called unconditionally, shouldn't the vector continue to grow unless an error is explicitly thrown?

[inclyc]

I just wrote the tests for you, no worries.

[Mr-Stoneman]

Ty I was struggling with that.

[inclyc]

The tests passed anyway however I think the real problem here is nixd/lib/Eval/AttrSetProvider.cpp where the options list is truncated but it does not notify the Controller the list is truncated.

nixd/nixd/lib/Eval/AttrSetProvider.cpp

Lines 366 to 368 in a6dfbbe

	// We set this a very limited number as to speedup
	if (Response.size() >= MaxItems)
	break;

[Mr-Stoneman]

Regarding the unconditionally growth, the AttrSetProvider.cpp file also has a constexpr int MaxItems = 30; which also impacts the behavior of this bug. As i currently understand it this Maxitems = 30 limits the available Names, that are used to fill the Items

[inclyc]

Regarding the unconditionally growth, the AttrSetProvider.cpp file also has a constexpr int MaxItems = 30; which also impacts the behavior of this bug. As i currently understand it this Maxitems = 30 limits the available Names, that are used to fill the Items

So maybe the correct fix is to add a isIncomplete field to AttrSetProvider.cpp though, the Controller is notified that the list it receives is also incomplete, HDYT?

[Mr-Stoneman]

Sounds reasonable, then you could remove the redundant MaxCompletionSize and only track this in the nixd/lib/Eval/AttrSetProvider.cpp file. Also some of the try catch could be removed as well. To tracking this as a value makes more sense than as a error.

[inclyc]

then you could remove the redundant MaxCompletionSize and only track this in the nixd/lib/Eval/AttrSetProvider.cpp file.

No, because not only attrset provider may add items into the completion list.

[Mr-Stoneman]

Okay didn't know that. So in that case the MaxCompletionSize would still be checked but now at the correct spot and there would be a additional isIncomplete field. But isn't the isIncomplete redundant because the MaxCompletionSize check would already catch this?