ImGuiListClipper.ForceDisplayRangeByIndices() documentation slightly misleading.

[JaedanC]

Version/Branch of Dear ImGui:

Version: 1.89.6 WIP (18954)
Branch: docking

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_glfw.cpp + imgui_impl_opengl3.cpp

My Issue/Question:

Hi Team. While looking to create a binding on top of dear_bindings I've been trying to create my own testing window similar to ShowDemoWindow, but in the binding language. This process involves wrapping a function and then looking for a way to implement the function to ensure the binding works. The actual code I'm running is the dear_bindings C version in the minimal example. This issue does not regard dear_bindings, but rather ImGui's documentation. This detail has been included for transparency.

The function this issue regards is:

imgui.h

    // Call ForceDisplayRangeByIndices() before first call to Step() if you need a range of items to be displayed regardless of visibility.
    IMGUI_API void  ForceDisplayRangeByIndices(int item_min, int item_max); // item_max is exclusive e.g. use (42, 42+1) to make item 42 always visible BUT due to alignment/padding of certain items it is likely that an extra item may be included on either end of the display range.

The documentation seems to imply that this function is forcing a specific range of items to be displayed (as in visible)? If you test that theory, it is incorrect. Based Upon GamingMinds-DanielC's minimal example from (the unfortunately hard to read) #5995 (working below):

        if ( ImGui::Begin( "Clipper Test" ) )
	{
		const int  count = 1000;
		static int index = 0;

		ImGui::SliderInt( "Index", &index, 0, count - 1, "%d", ImGuiSliderFlags_AlwaysClamp );

		const bool scrollTo = ImGui::IsItemDeactivatedAfterEdit();

		if ( ImGui::BeginChild( "clipped_list" ) )
		{
			ImGuiListClipper clipper;

			clipper.Begin( count );
			if ( scrollTo )
				clipper.ForceDisplayRangeByIndices( index, index + 1 );

			while ( clipper.Step() )
			{
				for ( int i = clipper.DisplayStart; i < clipper.DisplayEnd; ++i )
				{
					ImGui::Text( "Item #%d", i );
					if ( scrollTo && ( i == index ) )
						ImGui::SetScrollHereY(0.5f);
				}
			}
		}
		ImGui::EndChild();
	}
	ImGui::End();

We can see that what it actually does is force those indices to appear inside a call to clipper.Step(). This doesn't necessarily mean that those indices are visible. The example above shows this if you comment-out the SetScrollHereY. Without ForceDisplayRangeByIndices the call to SetScrollHereY never happens. My suggestion is to be more clear inside the documentation for anyone looking to use the function. The function is also not used in ShowDemoWindow so the comment and imgui.cpp is all we have.

I suggest an updated comment similar to:

imgui.h

    // Call ForceDisplayRangeByIndices() before first call to Step() if you need a range of items to be included inside one of the calls to Step() regardless of visibility.
    // This doesn't force the indices to become visible, but rather forces those indices to appear inside one of the calls to Step().
    // Can be useful for eg. calling ImGui::SetScrollHereY() on a out-of-view index so that it *does* become visible.
    IMGUI_API void  ForceDisplayRangeByIndices(int item_min, int item_max); // item_max is exclusive e.g. use (42, 42+1) to make item 42 always visible BUT due to alignment/padding of certain items it is likely that an extra item may be included on either end of the display range.

Or, I think it would be great if Daniel's example were added to the Demo Window to demonstrate the function's uses.

Maybe this is my lack of understanding regarding the terminolgy of "displayed" in ImGui.

This has been opened as an issue because I believe the current comment is a little confusing and hopefully will serve as a helper for others in the future regardless if the suggestion is adopted.

[GamingMinds-DanielC]

Yes, I can see how the "Display" in the Name can be misleading. A more appropriate name might be f.e. "IncludeRangeByIndices" (with the old name wrapping to the new one for compatibility, but marked as obsolete). If there is any renaming, I would suggest renaming the parameters to "item_begin" and "item_end" as well, since those are known widely from the STL and "end" is always excluded from the range, while "max" usually implies inclusion.

I originally needed to extend the list clipper for correct display of drag&drop information, even with items not displayed.

[JaedanC]

I agree with this. Renaming the function and parameters would be good too.

[ocornut]

You are right.
I was hesitating between UnclipRangeByIndices() and IncludeRangeByIndices().. I'm finding the earlier more charming but perhaps the later a little more comprehensive, so will use the later.

(Linking to #3841 for future ref)

[ocornut]

Pushed ecb0aaa, thanks both!

[GamingMinds-DanielC]

Pushed ecb0aaa, thanks both!

Thanks. just took a look at the commit and noticed a few minor things...

Parameter inconsistency:
Parameters are renamed to item_begin and item_end in imgui.h, but are still called item_min and item_max in imgui.cpp.

Copy & paste errors or typos in CHANGELOG.txt:

• "Use 'ImGuiListClipper() + clipper.Begin().clipper.Begin()'."
• "andrarely used"

[ocornut]

Thank you Daniel (that'll teach me for committing hastily before going to lunch). I've amended this with e489e40 and took the liberty to attribute you the commit.

[JaedanC]

What do you think about updating the comment too? I think the comment is alluding to the old name. Slighly modifying the comment I suggested above to be more in line with the current comment, it could be more verbose like so:

// Call IncludeRangeByIndices() before first call to Step() if you need a range of items to be included inside one of the calls to Step() regardless of visibility.
IMGUI_API void IncludeRangeByIndices(int item_begin, int item_end); // item_end is exclusive e.g. use (42, 42+1) to make item 42 always visible BUT due to alignment/padding of certain items it is possible/likely that an extra item may be included on either end of the display range.

[ocornut]

I'll further tweak the comment in a future commit, thanks.

[ocornut]

I have made two changes to those API now:

• Added single item version IncludeItemByIndex(int item_index). This is common with some things I'm working on and I didn't want to burden people with the range version.
• Renamed IncludeRangeByIndices() again.. to IncludeItemsByIndex(). Felt more consistent next to each others. Sorry!