[ wxwindows-Patches-1835458 ] Autogenerated ID reference counted

Thu Jan 3 14:54:34 PST 2008

Patches item #1835458, was opened at 2007-11-20 23:14
Message generated for change (Comment added) made by vadz
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=309863&aid=1835458&group_id=9863

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
>Status: Pending
Resolution: None
Priority: 5
Private: No
Submitted By: Brian A. Vanderburg II (brianvanderburg)
Assigned to: Nobody/Anonymous (nobody)
Summary: Autogenerated ID reference counted

Initial Comment:
This is the first patch for using reference counted ids.  The define wxUSE_AUTOID_MANAGEMENT is set to 1 to enable it, or 0 for the old way.  wxIdManger::NewId can generate an id or an array of ids.

wxUSE_AUTOID_MANAGEMENT is 0 by default, (because configure does not yet detect it and I don't know bakefile so I don't know how to have configure detect it), but should stay 0 by default even on MSW until I create a few more patches.

This code in windowid.cpp/wincmn.cpp compiles successfully when it is 0 or 1.  I've successfully compiled all wxWidgets with it set to 0 and tested out some samples and it worked fine, no problems, but it will not compile with it as 1 yet, until a few changes are made in the next patch.

Also, one thing is that the array of ids means that it needs to include dynarray.h (from within windowid.h in defs.h) which may not be desired.  A fix is to put wxArrayWindowID in dynarray.h/.cpp and take it by reference in the NewId function that supports creating it, and forward declare it as needed in windowid.h, so that defs.h does not have to include indirectly dynarray.h

The next patch will change things in MSW where it may fail to compile when wxUSE_AUTOID_MANAGEMENT is 1.  It will also change things in MSW to 'hold' the id, as is needed so it doesn't get released until the control that has it is finished (radiobox, menu item, tool item, common and generic code)

----------------------------------------------------------------------

>Comment By: Vadim Zeitlin (vadz)
Date: 2008-01-03 23:54

Message:
Logged In: YES 
user_id=71618
Originator: NO

Brian, this patch doesn't seem to include the most important parts:
windowid.h/.cpp (have you forgotten "-N" when doing the diff?). Could you
please redo it with them?

Also, I'd strongly prefer to have a single patch with all the changes
instead of having 2 of them in this entry and maybe some more in the other
3 entries for the same topic. Could you please make such comprehensive
patch and put it here and close all the other entries? Thanks in advance!

----------------------------------------------------------------------

Comment By: Brian A. Vanderburg II (brianvanderburg)
Date: 2007-12-13 05:08

Message:
Logged In: YES 
user_id=1033354
Originator: YES

This patch makes the changes as follows:

wxUSE_AUTOID_MANAGEMENT is the only define,  wxWindowIDRef is always an
object.  If wxUSE_AUTOID_MANAGEMENT is 0, then it is entirely inlined in
the header but if it is 1 then the 'Ref' function is in the source file and
simply deals with the reference count of the ID as needed and the rest of
it is still in the headers.  If it is 1, then the IDs are reference counted
as below but if 0, just decremented like normal in the range
wxID_AUTO_LOWEST..wxID_AUTO_HIGHEST,

wxWindowBase::NewControlId/wxIdManager::ReserveId finds a free id or range
of ids and reserves them for use, returning the first of the range or
wxID_NONE.

wxWindowBase::UnreserveControlId/wxIdManager::UnreserveId released
reserved ids that were 'not' assigned to a wxWindowIDRef.  Reserved ids
assigned to a wxWindowIDRef do not need to call this.

Assigning the id to a wxWindowIDRef increases the reference count of that
id (and decreases the reference count of the previous id if any).  If the
id was reserved, it will set the reference count to 1. The reference
counting is only done for ids in the auto-id range.  

wxIdManager is in the base library so any base code that needs it can use
it.  wxWindowBase::NewControlId/UnreserveId is in the gui library and just
calls the wxIdManager functions.
File Added: autoid_refcount_better2.patch

----------------------------------------------------------------------

Comment By: Vadim Zeitlin (vadz)
Date: 2007-12-11 20:41

Message:
Logged In: YES 
user_id=71618
Originator: NO

Brian,

I posted a question about this patch to wx-dev (see
http://article.gmane.org/gmane.comp.lib.wxwidgets.devel/95335) but you
apparently didn't notice it. I've also written to you directly but I'm not
sure if you received my email neither (AOL seems to be filtering email from
my domain). Please reply to the question in the message above when you
can.

Thanks!

----------------------------------------------------------------------

Comment By: Brian A. Vanderburg II (brianvanderburg)
Date: 2007-11-22 18:13

Message:
Logged In: YES 
user_id=1033354
Originator: YES

File Added: autoid_refcount_other.patch

----------------------------------------------------------------------

Comment By: Brian A. Vanderburg II (brianvanderburg)
Date: 2007-11-22 18:13

Message:
Logged In: YES 
user_id=1033354
Originator: YES

This patch replaces the first patch.  It takes a slightly different
approach.  Instead of wxWindowID being an object, it is an int.  Anywhere
there is an ID and you want to make sure that the ID is not created by
NewControlId/wxIdManager::ReserveId until you are done with it, the ID
should be assigned to a wxWindowIDRef.

If wxUSE_AUTOID_MANAGEMENT is 0, wxWindowIDRef is simple wxWindowID.  If
it is 1, then
wxWindowIDRef is a class with some conversions to make it almost an int. 
If wxUSE_FAKE_AUTOID_MANAGEMENT is 0, then the reference counting is used,
else it is just decremented, but still a class.

wxIdManager::NewId will mark the ID as 'reserved', but not currently in
use.  It just returns the value and not an object or array of objects. 
This makes it where one function  can be used to allocate just one id or
more than one, and wxArray is not needed.  Also it helps deal with code
that does 'int id = NewControlId', where the temporary returned object (if
it were done that way) may be destroyed before the id is ever
assigned/used/etc.

The return value should be assigned to a wxWindowIDRef. If it is passed to
a function or create method, that function should assign it to a
wxWindowIDRef (even if it is a create method that returns false) because
the user/code assumes that calling Create/etc with it will do that. 
Basically, the user should be able to assume that a window/menu/tool/et
constructor/create function will assign the id, even if it returns false:

If the ID is not assigned to a wxWindowIDRef, then it needs to be
unreserved.  This is very rare, but is done with wxIdManager::UnreserveId
(or wxWindow[Base]::UnreserveControlId)

Look at the code:

wxWindowID id = wxIdManager::ReserveId(5);

    for(int pos = 0; pos < 5; pos++)
    {
        MyControl* t = new MyControl(this, id++, ...);

        ..
    }

If ReserveId did not mark the IDs as reserved, and MyControl internally
uses ReserveId/NewControlId, then it may likely take some of the IDs that
were supposed to be used:

Like:

    MyControl::MyControl(...)
    {
        if(id == wxID_ANY)
            m_windowId = NewControlId();
        else
            m_windowId = id;

        // This is also safe since NewControlId/ReserveId return the value
and not an object
        // m_windowId = (id == wxID_ANY) ? NewControlId() : id;

        m_subItemId = NewControlId();
        ...
    }

Lets say up above (at wxIdManager::ReserveId(5)) that id is -32000.  If
the IDs are not marked as reserved, then the constructor will use ID -32000
(assigning it to m_windowID and increasing the count to 1) and then call
m_subItemId = NewControlId() which will return -31999, instead of -31995.
By marking the IDs as reserved, even though it is not yet used by a
wxWindowIDRef, it is ensured that the IDs -32000 to -31996 are not used.

If the ID is not used, then it needs to be unreserved:

    wxWindowID id = NewControlId(5);

    for(int i = 0; i < 5; i++)
    {
        MyControl* t = New MyControl(this, id++, ...);

        if(condition)
        {
            return false;
        }
    }

This would prevent the IDs from being reused since they are reserved. 
Instead it should be written as:

    wxWindowID id = NewControlId(5);

    for(int i = 0; i < 5; i++)
    {
        MyControl* t = New MyControl(this, id++, ...);

        if(condition)
        {
            UnreserveControlId(id, 5 - i - 1);
            // or wxIdManager::UnreserveId
            return false;
        }
    }

Or even simpler:

    for(int i = 0; i < 5; i++)
    {
        MyControl* t = New MyControl(this, NewControlId(), ...);

        if(condition)
        {
            return false;
        }
    }

Even though wxWindowIDRef is an object with conversions to an integer,
there may be some places where a cast is needed.  In the conditional ( q ?
a : b ), 'a' and 'b' need to be the same type:

    result = condition ? m_windowId : 0;

will cause a compiler problem if m_windowId is a wxWindowIDRef.

    result = condition ? (wxWindowID)m_windowId : 0;

will work fine.  Also in MSW code, there is no conversion to HMENU
(because HMENU may be void* in some cases but if STRICT is defined may be
something else).

    CreateWindow(..., (HMENU)m_windowId, ...)

will cause a compiler problem (if m_windowId is a wxWindowIDRef.

    CreateWindow(..., (HMENU)(wxWindowID)m_windowId, ...)

will work fine whether m_windowId is an object or a regular int.

Remember the rule: Anywhere there is an ID and you want to make sure that
the ID is not created by NewControlId/wxIdManager::ReserveId until you are
done with it, the ID should be assigned to a wxWindowIDRef.

If there is code that looks like:

    for(...)
    {
        CreateWindow(..., (HMENU)NewControlId(), ...)
    }

It causes a problem because the ID is marked as reserved by NewControlId,
but will never be released since it is not being stored in a wxWindowIDRef.
 It is best to store the items in an array:

    m_ids = new wxWindowIDRef[n];
    for(...)
    {
        m_ids[i] = NewControlId();
        CreateWindow(..., (HMENU)(wxWindowID)m_ids[i], ...)
    }

then in the destructor, just delete the array to decrease the reference
count to the ids:

    delete [] m_ids;

This is true for any port that wants to be compatible with
wxUSE_AUTOID_MANAGEMENT=1/wxUSE_FAKE_AUTOID_MANAGEMENT=0.

Simply put, NewId reserves the id but does not use it yet.  If it is
assigned to wxWindowIDRef, then it is used and will be released/decreases
when the wxWindowIDRef is destroyed or the id changes.  But if it is not
assigned to wxWindowIDRef, then there is no way to 'unreserve' it so it
must be manually unreserved.

Possible wxPython issues:

ID_FOO = wx.NewId()

ID_FOO can be just an int.  It will be reserved and when the object gets
created, it will (inside wxWidgets) be assigned to a wxWindowIDRef.  But if
it is not used, it will need to be released.  A function wx.UnreserveId
could be used for this, but may be difficult to work with the globals
(ID_FOO).  This wouldn't be a problem for a pure wxPython program, because
the global ids will exist until app shutdown so it doesn't matter if they
are reserved, but if wxPython is embeded and the code gets loaded from a
script over and over each execution of the script, then ID_FOO=wx.NewId()
,may reserve the ID but not release it when it is done if it is not used,
and cause id 'leaks' over and over.

If instead wx.NewId returned a wrapper for wxWindowIDRef, then this would
not be a problem at all because it would be marked as reserved but then
assigned to a wxWindowIDRef.  When the wrapper is destroyed the ref count
would be decremented like it should, but the wrapper must be able to
convert so that txt = wx.TextCtrl(self, ID_FOO, would still work whether
ID_FOO is a numeric value directly or a wxWindowIDRef wrapper.

This patch only makes the changes to compile fine with
wxUSE_AUTOID_MANAGEMENT=0.  The next patch is needed to make some changes
to a few other files to work right when wxUSE_AUTOID_MANAGEMENT=1

File Added: autoid_refcount_better.patch

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=309863&aid=1835458&group_id=9863