Fixed nTop not being incremented in ListBox:hitTest() for no-box dropdown

[kamilprzyb2]

2025-01-08 13:14 UTC+0100 Kamil Przybylski (kprzybylski quay.pl)

* src/rtl/listbox.prg
! fixed nTop not being incremented in ListBox:hitTest() for no-box dropdown
* replaced inconsistent references to ::nTop with nTop
* renamed variables to better reflect their role

Explanation:

• With current logic, nTop in hitTest() was incremented for a dropdown only if it had boxes.
  This caused no-box dropdowns to be off by 1 in calculations.
  

  core/src/rtl/listbox.prg

  Lines 411 to 416 in 56bd635

  	IF ! ::lIsOpen .OR. Empty( ::cHotBox + ::cColdBox )
  	nRet := 0
  	ELSE
  	IF ::lDropDown
  	nTop++
  	ENDIF

• As suggested by @alcz in Fix nTop not being assigned properly in ListBox:hitTest() #371 the function referenced both ::nTop and nTop inconsistently. I changed all referenced to nTop.
• Renamed variable nRet to nOffset, since it is misleading and isn't even returned by the function.
• Changed last line RETURN 0 to RETURN HTNOWHERE to be more consistant and descriptive.

[alcz]

Hey! Isn't one-off for dropdown somehow intentional? Do you have some test-code I could run against Cl*pper?

[kamilprzyb2]

Yes the dropdown-specific shift is intentional, I have not removed it, but I modified the condition to make sure no-box dropdown is also affected (which is not the case currently).
Here is a snippet demonstrating the issue

#include "inkey.ch"
FUNCTION DropdownTest()

	LOCAL GetList := {}, loGet

	LOCAL laItems := {"One", "Two", "Three", "Four", "Five", "Six"}
	LOCAL lcItem := "One"
	LOCAL lcItem2 := "Two"
	LOCAL lcColor := "W/N,N/W,N/W,W+/R,W/N,R+/N,G+/N,W+/R"

        SET EVENTMASK TO INKEY_ALL - INKEY_MOVE // 254

	CLS

	@ 10, 5, 15, 25 GET lcItem LISTBOX laItems DROPDOWN  COLOR( lcColor )
	@ 17, 5, 22, 25 GET lcItem2 LISTBOX laItems DROPDOWN COLOR( lcColor )

	loGet := ATAIL( GetList )
	loGet:Control:hotBox := ""
	loGet:Control:coldBox := ""

	READ

	? lcItem, lcItem2

	RETURN ( NIL )

Listbox1 acts as expected, but Listbox2 (no-box) is acting drunk
Clicking on item one selects item two, clicking on two selects three, and so on.

[alcz]

I've redacted your example adding inkey EVENTMASK, in case someone else wants to test.

Guess what, testing on Cl*pper 5.3 shows that borderless dropdown mouse was buggy there as well.

IMHO replicating such crash-bug even under HB_CLP_STRICT doesn't make sense, but your code would probably never run under the old ancestor compiler. Though may qualify for a NOTE/ comment/explanation in the code itself.

For C5.3 test one may want to call MSetCursor( .T. ) too, but that depends if hardware/VGA cursor is available (i don't know).

All this means your patch is very good! Thanks for reviewing this rarely used widget from Harbour codebase.

[alcz]

If you could rewrite this line to be IF ::lIsOpen .AND. ! Empty( ::cHotBox + ::cColdBox )
and add some comment/note that here we're extending over Cl*pper's abilites, then I think your patch is okay to be pulled in.

[kamilprzyb2]

Thank you.

The crash looks very similar to the issue I brought up in #371. Clicking anywhere on a borderless dropdown triggers this exact error. Perhaps it was somehow inherited from Cl*pper 🤔
I suppose it wasn’t common to use no-box dropdowns with mouse combos, but this issue occasionally causes crashes in a Harbour app I’m maintaining. Feels great to see this issue resolved.