Problems setting bitmaps to Static Control MFC

Question

I'm currently fighting with a dialog based GUI, while trying to customize it. I have drawn some bitmaps which I would like to use as buttons and as logos in the dlg.

I have written two member functions, one for setting bitmaps to CButtons, and one for setting bitmaps to CStatics. Actually both are working, when calling them on button press. But only the member-function for setting the CButtons works properly during the initialisation of the dialog. My CStatics are getting overwritten somehow.

This is working when called in OnInitDialog()

void CMyDlg::setBitmapAsButton(std::string file, CButton &Button, int xPos, int yPos)
{
    CImage imgFromFile;
    CBitmap bitmap;
    std::wstring wfile(file.begin(), file.end());

    HRESULT ret = imgFromFile.Load(wfile.c_str());
    int width = imgFromFile.GetWidth();
    int height = imgFromFile.GetHeight();
    bitmap.Attach(imgFromFile.Detach());

    Button.Create(_T("My button"), WS_CHILD | WS_VISIBLE | BS_BITMAP,
    CRect(xPos, yPos, xPos + width, yPos + height), this, 1);
    Button.SetBitmap(bitmap);
}

This is NOT working when called in OnInitDialog():

void CVisTrayDlg::setBitmapAsStatic(std::string file, CStatic &Static, int xPos, int yPos)
{
    CImage imgFromFile;
    CBitmap bitmap;
    std::wstring wfile(file.begin(), file.end());

    HRESULT ret = imgFromFile.Load(wfile.c_str());
    int width = imgFromFile.GetWidth();
    int height = imgFromFile.GetHeight();
    bitmap.Attach(imgFromFile.Detach());

    Static.Create(_T("My Static"), WS_CHILD | WS_VISIBLE | SS_BITMAP,
    CRect(xPos, yPos, xPos + width, yPos + height), this);
    Static.SetBitmap(bitmap);
}

CButton and CStatic controls are declared as member of CMyDlg.

Any ideas on how I can avoid this behaviour? Or is there even a better way to place bitmaps in a dialog? (I tried CImage::BitBlt() which got also overwritten somehow.)

I believe `CBitmap` needs to exist after function returns. Try making it a member variable (or use `static` keyword for quick results within this function). — Ajay, Jun 06 '16 at 10:47
@Ajay yes, you are right. Declaring the CBitmaps as member variables works. Strange... For CStatic controls it's necessary but not for CButton controls. Thank you very much! — GregPhil, Jun 06 '16 at 11:33
@Ajay I adapted your suggested code, witch works fine, but somehow the memory is not freed correctly. Maybe you have a second to crosscheck it: http://stackoverflow.com/questions/38118923/how-to-free-cwin-objects-created-in-a-child-diaolog-to-avoid-memory-leaks — GregPhil, Jul 02 '16 at 16:04

IInspectable · Accepted Answer · 2016-06-06T12:37:34.613

The issue is one of ownership: After the call to SetBitmap returns, the single bitmap instance has 2 owners, the CBitmap object as well as the CButton/CStatic¹⁾ control. When the CBitmap object goes out of scope, it destroys the single instance, leaving the control with a handle to an invalid bitmap.

The standard solution is to explicitly pass ownership, by invoking CImage::Detach() before passing the HBITMAP off to the button or static control:

void CVisTrayDlg::setBitmapAsStatic(std::string file, CStatic &Static, int xPos, int yPos)
{
    CImage bitmap;
    // Convert from ANSI codepage to UTF-16
    CStringW wfile(file.c_str());

    // Error handling needed. Use the SUCCEEDED() or FAILED() macros here:
    HRESULT ret = bitmap.Load(wfile);
    int width = bitmap.GetWidth();
    int height = bitmap.GetHeight();

    Static.Create(_T("My Static"), WS_CHILD | WS_VISIBLE | SS_BITMAP,
                  CRect(xPos, yPos, xPos + width, yPos + height), this);
    // Pass ownership from CImage to CStatic:
    HBITMAP hbmOld = Static.SetBitmap(bitmap.Detach());
    // SetBitmap() passes ownership of the previous image (if set).
    // We are responsible for cleanup:
    if (hbmOld != nullptr ) {
        ::DeleteObject(hbmOld);
    }
}

A few notes on the implementation:

Your conversion from string to wstring fails unless file uses ASCII characters only. I changed the code to deal with ANSI codepage encoding, but that may not be the source encoding. You may have to change this.²⁾
The conversion from CImage to CBitmap is not needed. CImage has both an operator HBITMAP() as well as a Detach() member, and that's all we really need here.
The call to SetBitmap returns a handle to the previously associated bitmap. We are responsible for cleanup.

¹⁾ There is no documented contract, that buttons and static controls behave differently. If it appears to work for buttons, then it does so by coincidence only. It's not anything you should rely on. It is recommended to use the same solution for buttons as well as static controls.

²⁾ Windows uses UTF-16 throughout. It's usually best to use UTF-16 as your application's internal character encoding as well. Using std::wstring is recommended.

I'm speechless, it works like charms and your explanation is just perfect! Everything just make sense to me now! — GregPhil, Jun 06 '16 at 17:32
I'm using your suggested approach while opening a child dialog to load several images. Because I realised a memory leak I checked the number of GDI objects where I can see a significant raise of the objects by the amount of loaded images while opening the child dialog, but no decrease after closing it. I have opened a new thread for this topic:http://stackoverflow.com/questions/38118923/how-to-free-cwin-objects-created-in-a-child-diaolog-to-avoid-memory-leaks. Any idea what is missing? — GregPhil, Jul 04 '16 at 09:25
It's even worse: https://devblogs.microsoft.com/oldnewthing/20140219-00/?p=1713. The static control does not have ownership and you must also delete a shadow copy of the bitmap. — gast128, Jan 30 '20 at 16:48
@gas: That's *really* bad. I'll see if I can incorporate that to meet the requirements of a mad design. It'll be a while until I get around doing that. — IInspectable, Feb 01 '20 at 12:35

score 2 · Answer 2 · answered Jun 06 '16 at 11:43

2

Ensure that CBitmap object remain in memory. Hence, make is member variable of your class. The destructor of dialog class will eventually call the destructor of bitmap objects, so no need to worry about resource/memory leak.

For button it may show its effect when you move/restore the window. I would advice you to keep CBitmap object for button(s) also in memory.

answered Jun 06 '16 at 11:43

Ajay

18,086
12
59
105

This fails in case the user calls [CWnd::Detach](https://msdn.microsoft.com/en-us/library/7zhdchw8.aspx) and destroys the C++ class prior to the `HWND` for any reason. You'll find the correct solution [here](http://stackoverflow.com/a/37657413/1889329), implementing clean ownership semantics. – IInspectable Jun 06 '16 at 12:47

Problems setting bitmaps to Static Control MFC

2 Answers2

Linked