#182 OODialog List Control bug

3.2.0
closed
None
5
2012-08-14
2006-09-05
Jon Wolfers
No

Open Object Rexx Interpreter Version3.1.0
Build date: Aug 15 2006
Windows XP Home v2002 SP2

This behaviour also existed in 3.0, however it was
brought to my attention by the decision to use the
lastItem attribute to fix the listControl next/previous
bug.

Using GetListControl method of AdvanceControls class
appears to set ListControls LastItem Attribute to 0

So if I have

========================
::method InitDialog

CURLIST = SELF~GETLISTCONTROL(100)
curlist~prepare4nitems(data~items)
do data~items
curlist~addRow(,,data~text)
data~next
end
SAY ‘INITDIALOG’ CURLIST~LASTITEM CURLIST~ITEMS

::method otherMethod
CURLIST = SELF~GETLISTCONTROL(100)
SAY ‘OTHERMETHOD’ CURLIST~LASTITEM CURLIST~ITEMS
========================

I see
InitDialog 805 806
OtherMethod 0 806

Whereas if I have

========================
::method InitDialog
EXPOSE CURLIST

CURLIST = SELF~GETLISTCONTROL(100)
curlist~prepare4nitems(data~items)
do data~items
curlist~addRow(,,data~text)
data~next
end
SAY ‘INITDIALOG’ CURLIST~LASTITEM CURLIST~ITEMS

::method otherMethod
EXPOSE CURLIST
SAY ‘OTHERMETHOD’ CURLIST~LASTITEM CURLIST~ITEMS
========================

I see
InitDialog 805 806
OtherMethod 805 806

Looking at the Class code I can’t see how or why this
happens

This causes iterations through listcontrols to fail
when method 1 is used.
It is not always an option to use the expose method as
you might want to apply a method to different
listControls (ie: sorting by column) so want to pass the id

I attach a small program that shows the value of the
list control LASTITEM attribute when the object is Got
(method 1) or when the object is exposed (method 2)

Jon

Discussion

  • Jon Wolfers

    Jon Wolfers - 2006-09-05

    demonstrates Listcontrol error

     
  • Gert van der Kooij

    Logged In: YES
    user_id=685845

    Hi Jon,

    If you change LASTITEM to LAST it will work.

    Kind regards, Gert

     
  • Jon Wolfers

    Jon Wolfers - 2006-09-07

    Logged In: YES
    user_id=667060

    I'm not sure what you mean Gert, do you mean if the
    ListControl Previous, Next, NextLeft & NextRight methods
    were changed to read
    if self~Last < 2 then return -1
    instead of
    if self~LastItem < 2 then return -1
    in which case, I agree with you that that would obviate the
    listControl endless iteration problem.

    When I put the patch in for this problem, I chose LastItem
    because it was an attribute and did not require a call to
    the HandleListControl function and would thus run faster.

    However, changing those methods would not fix the underlying
    bug with lastitem, which I do not understand. When you
    retrieve the ListControl object using the AdvancedDialogs
    GetListControl Method it reruns the listControls initmethod
    which contains the line self~lastItem = 0.

    Obviously that is apropriate if this is a newly instantiated
    list, but not if you are merely retieving a previously
    instantiated list.

    The parameters to the init method do not give a clue as to
    whether this is an instantiation or a retrieval.

    However, I cannot understand how the code works to retrieve
    a list so cannot suggest a fix for this.

    Jon

     
  • Gert van der Kooij

    Logged In: YES
    user_id=685845

    Hi Jon,
    what I mean is that you are using an internal, not
    documented attribute. What's happening with that atribute is
    unknown so you can't rely on it the way you use it in your
    test script.

    I wasn't talking about the way LASTITEM is used within the
    listcontrol class, that's internal as far as I know.
    Which bug nr. are you trying to solve, I couldn't find it.

     
  • Jon Wolfers

    Jon Wolfers - 2006-09-08

    Logged In: YES
    user_id=667060

    Hi Gert,
    I don't have a lot of time today - just off on holiday.
    The original bug was 1165949
    I supplied the solution which proved not to work in
    circumstances where the iteration through the listcontrol
    occured on an instance of the listcontrol which had been
    retrieved using the GetListControl method, but works fine
    when the original definition of the list control is in
    scope (or presumably when the listControl instance has been
    passed to the routine).
    It seemed reasonable to me to use an 'internal' attribute in
    my patch as my patch was to be part of the class.
    My test program accesses that 'internal' attribute because
    the purpose of that test program is to demonstrate the bug
    which affects that internal attribute.

    I don't have time before I go to write the code to
    demonstrate the consequences of that bug, but if you use
    this code

    list = Self~GetListControl(id)
    this_item = 0
    do while this_item \= .nil
    say list~itemText(this_Item)
    this_item = list~next(this_item)
    end

    you will only ever see one item of the list

    Jon

     
  • Gert van der Kooij

    Logged In: YES
    user_id=685845

    Hi Jon,
    Correct me if I'm wrong but it seems like the problem is
    caused by the GetControl method which is called by the
    GetListControl method. It creates a new instance of the
    ListControl class and the Init method of that class sets
    LASTITEM to 0.
    Most likely no new instance of the ListControl is created
    but instead of that a pointer to the allready existing
    instance is returned, which might be done by the 'super' class.

    Maybe changing 'self~Lastitem = 0' on line 679 in advctrl.cls to

    if (LastItem = .Nil) then
    Lastitem = 0

    would solve your problem.

    Kind regards, Gert

     
  • Gert van der Kooij

    Logged In: YES
    user_id=685845

    Forget the 'solution', that wasn't tested and doesn't work
    at all.

     
  • Jon Wolfers

    Jon Wolfers - 2006-09-17

    Logged In: YES
    user_id=667060

    Hi Gurt,

    yes, your analysis coincides with mine. I don't understand
    how to tell if this is a new instance of the listcontrol
    class or not.

    probably the best thing to do is to make the fix for 1165949
    use the items method or the last method instead of the
    lastitem attribute.

    My guess is that the lastItem attribute was somenes former
    attempt to fix this problem which also didn't work, but the
    code got left in.

    Jon

     
  • Gert van der Kooij

    Logged In: YES
    user_id=685845

    Hi Jon,

    I tried to find out where LastItem was used in the ooRexx
    code and it looks like the LastItem method/attribute has
    been mixed up.
    The ListClass.cpp defines the method LastItem with the
    following comment:

    /* Function: Return last item (value part) in the list

    I don't know if the definition of a LastItem attribute in
    advctrls.cls does create problems but it doesn't look right.
    Renaming it looks necessary. I also don't know why the
    LastItem method isn't documented, it looks 'available' for
    public use.

    Maybe someone of the IBM guys can look into the old Object
    Rexx code to find out where the LastItem attribute in
    platform\windows\oodialog\advctrls.cls was introduced?

    Regards, Gert

     
  • Jon Wolfers

    Jon Wolfers - 2006-09-20

    Logged In: YES
    user_id=667060

    Im busy at work so not had much time to look at this, but it
    seems that this bug also affects the ListControl addRow
    method where the default parameter is taken for the first
    argument, adding the row at the start of the list instead of
    the end.

    I had thought that substituting
    self~LastItem = self~Items-1
    for
    self~LastItem = 0
    in the init method of the listcontrol class might fix it,
    but for some reason I dont understand it causes an error in
    the external HandleListControl routine

     
  • Jon Wolfers

    Jon Wolfers - 2006-10-24

    Logged In: YES
    user_id=667060

    Here is a patch for the ooDialog advctrl.cls. It will not
    fix the underlying problem with the LastItem method, but it
    will cure the symptoms and will mean that code which worked
    in v3.0.0 but not in v3.0.1 will once more work.

    Rick, David, I understand there is a release soon & would
    ask you to include this tiny cange if you can. If there is
    any work I can do to support getting this included please
    let me know. I haven't been about much as we are one man
    short at work and I have been working all hours to cover.

    Jon

    Left file: C:\Documents and Settings\Sahananda\Desktop\Copy
    of advctrl.cls
    Right file: C:\Documents and
    Settings\Sahananda\Desktop\advctrl.cls
    9c9
    < / http://www.oorexx.org/license.html
    /


    / http://www.oorexx.org/license.html
    /
    900,901c900,902
    < -- a -1 value to mark the iteration end.
    < if self~LastItem < 2 then return -1


    -- a -1 value to mark the iteration end.
    [1165949]
    -- As the internal attribute LastItem is not reliable
    use the items method [1552754]
    if self~Items < 2 then return -1
    909,910c910,912
    < -- a -1 value to mark the iteration end.
    < if self~LastItem < 2 then return -1


    -- a -1 value to mark the iteration end.
    [1165949]
    -- As the internal attribute LastItem is not reliable
    use the items method [1552754]
    if self~Items < 2 then return -1
    918,919c920,922
    < -- a -1 value to mark the iteration end.
    < if self~LastItem < 2 then return -1


    -- a -1 value to mark the iteration end.
    [1165949]
    -- As the internal attribute LastItem is not reliable
    use the items method [1552754]
    if self~Items < 2 then return -1
    927,928c930,932
    < -- a -1 value to mark the iteration end.
    < if self~LastItem < 2 then return -1


    -- a -1 value to mark the iteration end.
    [1165949]
    -- As the internal attribute LastItem is not reliable
    use the items method [1552754]
    if self~Items < 2 then return -1

     
  • Rick McGuire

    Rick McGuire - 2006-10-24

    Logged In: YES
    user_id=1125291

    Jon, this is not in a form I'm able to apply to the code.
    Could you please generate the patch using "cvs diff" and add
    it as an attachment rather than inline in the comment?
    Alternatively, just append the corrected version of the file
    and I'll do the diffing myself.

    Rick

     
  • Rick McGuire

    Rick McGuire - 2006-10-24

    Logged In: YES
    user_id=1125291

    Jon, I managed to apply this by hand and have committed the
    changes. Could you double check I got this applied correctly?

    Rick

     
  • Jon Wolfers

    Jon Wolfers - 2006-10-24

    Logged In: YES
    user_id=667060

    Hi Rick,

    Yes, you made the changes perfectly. I'm sorry to still be
    such a clutz with the CVS stuff.

    That should be the end of the OODialog iterating over
    listcontrols problem.

    I don't know whether you should close this bug yet, because
    there is still the underlying error in the code for the
    lastItem private attribute, but whether that will ever
    affect anyones code I don't know. I'll leave it as your call.

    Thanks for your help, and all your work. Hope the dog is OK
    too.

    Jon

     
  • Mark Miesfeld

    Mark Miesfeld - 2007-02-17

    Demonstrates ooRexx ListControl objects are separate objects

     
  • Mark Miesfeld

    Mark Miesfeld - 2007-02-17

    Logged In: YES
    user_id=191588
    Originator: NO

    There was a bug in the lastItem code. This is fixed in CVS. Note that the lastItem attribute is not a private attribute - it is an undocumented attribute, it is not private.

    Jon, I also believe that you have a misconception about how GetListControl works. Each time GetListControl is called a new ooRexx object is created. If GetListControl is called with the same ID, in the same dialog, the new ooRexx object is connected to the same underlying Windows OS List Control, and there is only one of those. But, each ooRexx ListControl object is a separate object.

    The code to initially set the lastItem attribute was incorrect, it did not take into account that it could be connecting to an existing Windows List Control that already had items in it. That is fixed.

    However, if you create several ooRexx ListControl objects and keep references to them, and add items to the List, the lastItem attributes in the individual ooRexx objects will get out of sync. Jon, I am attaching a program to demonstrate that, based on your original example.

    The items method will always be correct, no matter how many ooRexx ListControl objects you create, because that method makes a call into the real Windows OS List Control to get the number of items.

    The code that maintains the lastItem attribute is contained within the ooRexx object. This means, if an ooRexx programmer creates several ooRexx ListControl objects for the same underlying Windows OS List Control, there is no way for the lastItem attribute to remain correct in all the objects (if the programmer adds items to the List Control.)

    Looking through the ListControl class code, some of it does not look fundamentally correct to me. It certainly seems to assume that no programmer would ever call methods on more than one ooRexx ListControl object connected to the same Windows OS List Control. But, as you've demonstrated, programmers certainly will. <grin>

    This looks like something that could benift from being reworked in 4.0.
    File Added: LastItemDemo.rex

     
  • Jon Wolfers

    Jon Wolfers - 2007-02-19

    Logged In: YES
    user_id=667060
    Originator: YES

    Hi Mark,
    thank you for that - I certainly did misunderstand and I think I was not the only one. I think some text needs to go in the ooDialog reference Advanced Controls section making it clear that OODialog requires that only one Get method should be performed on each control.

    I assumed that the get method returned the handle to the object, not created the object.

    I have coded these wrongly hundreds of times.

    I'm a bit deluged at the moment, but in a while, if nobody else has done so, I'll draft something for the manual.

    Thanks for fixing the lastitem bug.

    Jon

     

Anonymous
Anonymous

Cancel  Add attachments