Re: [CDK] A few problems, remarks and suggestions, mostly concerning the Scroll widget

[Thomas Dickey]

On Wed, Dec 26, 2018 at 03:34:12PM +0100, Stéphane Goujet wrote:
> Hello,
> 
> 
>   Here is a list of a few problems I found while using cdk-5.0-20180306:
> 
>   For the first 2 cases (bug problems), I haven't written test cases, but if
> you request it, I shall do it.
> 
> 
> 
> 1. In "scroll.c", function allocListArrays(), around line 714:
> 
> You free "item" but not the content of the list it points to.
> 
> Instead of:
> 
>       freeChecked (scrollp->item);
> 
> shouldn't it be something like:
> 
>       CDKfreeChtypes (scrollp->item);
> 
> as it is done in _destroyCDKScroll() ?

offhand - no: the data has just been copied to a new array.
What's being freed is the obsolete array.

> 2. If I happen to focus on an empty scroll list, my program crashes.
> 
> In "scroll.c", function drawCDKScrollCurrent(), around line 527: the access
> to
> 
>      s->itemPos[s->currentItem]
> 
> crashes because itemPos was not allocated, for createCDKScrollItemList()
> doesn't allocate anything if listSize is 0.
> 
> Wouldn't it be safer if the content of the function drawCDKScrollCurrent()
> was encapsulated in a:
> 
>       if(s->listSize > 0) {
>             /* Current function content */
>       }

that looks like an improvement (thanks)
> 
> ?
> 
> drawCDKScrollList() already has this protection.
> 
> 
> 
> 
> 3. Documentation (man cdk_scroll)
> 
> The code says getCDKScrollCurrent() is obsolete, but the man page does not.
> The man page doesn't either highlight the fact that getCDKScrollCurrent()
> and getCDKScrollCurrentItem() are the same.

agreed
 
> I guess something like "getCDKScrollCurrent() is obsolete, please use
> getCDKScrollCurrentItem(), same usage" would be better.
> 
> By the way, I prefer the description of getCDKScrollCurrent(), with "index",
> because the one of getCDKScrollCurrentItem(), with "number", may confuse me
> into believing it is about getting the number of items, the items count.
> (But I am not an English native speaker.)

ok :-)
 
> 4. Documentation and code (man cdk_scroll; "scroll.c", line 867)
> 
> getCDKScrollItems(): the man page doesn't mention that NULL is allowed for
> the list parameter, and that it is a way to get the number of items in the
> scroll list.

I added the null-pointer check in 2005, apparently after seeing something
break.  I suppose documenting it would help :-)
 
> BTW, I think it is (is it?) the only way to get the number of items, which
> is weird. A dedicated function would be much more natural.
> 
> 
> About the behaviour of this function, don't you find it weird that it
> doesn't allocate the proper list for you? So you have to, first, get the
> number of items (and that means calling this very same function with a NULL
> parameter), then allocate yourself a list withe the proper size, and then
> call the fucntion again with that list. It looks unnatural to me, I don't
> know... (I wondered a while before figuring out how it could be used.) Now
> of course for compatibility reasons, this last point cannot be changed.
> 
> 
> 
> 
> 5. Documentation (man cdk_scroll)
> 
> getCDKScrollCurrentTop() appears twice in the list of functions. Once at its
> rightful place, and then again in the place where the corresponding setter
> setCDKScrollCurrentTop() should be.

ok :-)
 
> 6. Feature (cdk_scroll)
> 
> There is no function such as replaceCDKScrollItem(). And emulating it in
> user code is ugly:
> 
> * save currentItem index,
> * modify currentItem index by accessing the Scroll structure directly,
> * call insertCDKScrollItem(), which only operates at currentItem index and
> not at a given position,
> * restore currentItem index by accessing the Scroll structure directly,
> * call deleteCDKScrollItem() to delete the following element.
> 
> And those calls mean moving data unnecessarily (and potentially reallocating
> memory).

ok...
 
> 7. Feature (cdk_binding)
> 
> getcCDKObject() is deprecated (well, there is a small typo "depcrecated" :-)
> )

I do _know_ how to spell that, but it's a frequent typo :-)
 
> getchCDKObject() replaces it, but it has an extra argument: boolean
> *functionKey, and this pointer is made mandatory.
> 
> What if I don't care about this functionKey indicator? Don't you think I
> should be allowed to pass NULL to mean "don't send me back this indicator" ?
> 
> This would just requested to insert a test into the function
> getchCDKObject() ("binding.c", line 228) before setting the indicator:
> 
>    if(functionKey) { //  <--- add this test
>        *functionKey = (key >= KEY_MIN && key <= KEY_MAX);
>    }

ok :-)

> 8. Well, no 8th point, I guess that's enough for now :-)

thanks

-- 
Thomas E. Dickey <address@hidden>
https://invisible-island.net
ftp://ftp.invisible-island.net

signature.asc
Description: Digital signature