[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Stéphane Goujet
Subject: [CDK] A few problems, remarks and suggestions, mostly concerning the Scroll widget
Date: Wed, 26 Dec 2018 15:34:12 +0100 (CET)
User-agent: Alpine 2.21.1 (LNX 216 2017-09-19)


  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


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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]