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

[Stéphane Goujet]

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() ?




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 */
      }

?

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.

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.)




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.

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.




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).




7. Feature (cdk_binding)

getcCDKObject() is deprecated (well, there is a small typo "depcrecated" 
:-) )

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);
   }




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




Faithfully yours,
  Stéphane Goujet.