#547 MDI menu merging does not preserve bitmaps

[Vidar Hasfjord]

For MDI applications, if a MDI child window has a menu descriptor, then when the child is activated, its menu is merged with the main menu. However, this process does not preserve menu item bitmaps, so if such have been assigned to the main menu, they are lost.

For an example, see the Classes example on the trunk, which recently got menu item bitmaps assigned [r6381][r6382][r6384]. Build and run it, then open the Examples submenu on the main menu. Note that many items have bitmaps (icons). Select "TTreeView". An MDI child opens displaying the TTreeView example. Open the Examples submenu on the main menu again. Note that the menu item bitmaps are gone.

PS. Why do many of the MDI children in the Classes example have empty dummy menu descriptors? These seem superfluous. Note that without a menu descriptor, there is no menu merging, hence no triggering of the issue described in this ticket.

[Ognyan Chernokozhev]

Unfortunately, there does not seem to be a way to get the bitmaps for existing menu items, and thus to copy them to the new menu items.

A workaround is to override TFrameMenu::MergeMenu and re-assign the menu bitmaps after merge.

[Vidar Hasfjord]

@jogybl wrote:

Unfortunately, there does not seem to be a way to get the bitmaps for existing menu items

Shouldn't TMenu::GetMenuItemInfo do the trick?

See the the section on Menus in the Windows API documentation, and in particular the underlying function GetMenuItemInfo and its MENUITEMINFO structure (which is wrapped in OWLNext by TMenuItemInfo). The member hbmpItem should hold the bitmap handle, I guess.

I'm not sure what the handle ownership situation is here, though. This needs to be clarified, I would think, to decide whether to copy or move.

It would nice to have this bug fixed in the update. Thanks for giving it attention.

[Ognyan Chernokozhev]

I tried to use GetMenuItemInfo, but it did not return the bitmap handle. I think that the hbmpItem member will be populated if the menu item itself is a bitmap and not text. But for the menu to have both text and an icon, what is set is the 'checked' bitmap and I could not find a way to retrieve that. There is no corresponding Get function to SetMenuItemBitmaps.

Also, after sleeping on this, I think now that re-assigning the menu bitmaps after each menu merge or restore IS the correct approach - because some menu items may be added and removed, so a DeepCopy is not enough to keep the bitmaps across various actions. For example, in the classes samples, showing TBitmapView child removes CM_OPENWINDOW item from the Files menu, and there will be no easy way to restore it's bitmap after that view is closed.

[Vidar Hasfjord]

@jogybl wrote:

I tried to use GetMenuItemInfo, but it did not return the bitmap handle.

I had the same problem, read along! It is more baffling than you think!

I have spent all of today working on a solution, and although it had me stumped for many hours, I finally understand what is going on now.

TMenu::GetMenuItemInfo does work, but for a long time I didn't get any bitmaps returned when applying it within TMenu::DeepCopy (the core operation when merging menus), whatever flags I set, and whatever method I used to retrieve info.

(Retrieving info can also be done with TMenuItemInfo constructors, so I wasted a lot of time studying these, looking for issues, some of which I found, though unrelated — anyway, these constructors are headed for the scrapheap.)

Then enlightenment struck, and I realised that the Classes example puts the bitmaps on the active displayed menu. The problem is that the active menu is ephemeral — it is thrown out when the menu is updated. The key observation here is that the menu is synthesised by merging menu descriptors (menu templates).

So, for permanence, we need to set the bitmaps on the source menu descriptors instead. However, since we cannot get a mutable reference, we need to make a copy, modify it by adding bitmaps, and then set it as the TFrameWindow's new descriptor.

To avoid all this copying, I've added a non-const overload of GetMenuDescr [r7248].

I also discovered that TFrameWindow doesn't destroy the menu handle at cleanup, although it seems to take ownership (see DestroyMenu calls throughout "framewin.cpp"). Hence, I've added cleanup in CleanupWindow [r7247], which as a counterpart to TMenu creation and SetMenu in SetupWindow seems to make sense. But please review this. I may have overlooked something essential.

There was also an issue with self-assignment in SetMenuDescr [r7246].

With this in place, my bitmap copying code in TMenu::DeepCopy started working, and to my delight, Classes now works fine without your workaround.

An important unsolved issue remains though: Bitmap ownership. The bitmap handles assigned to the menu are not owned by the native Windows menu element. The application is responsible for destroying them. Since TMenu is copyable, it is probably best to make TMenu take ownership and destroy any bitmaps before destroying the menu handle (provided ShouldDelete is set to AutoDelete). In turn, that means that TFrameWindow propably should use a TMenu to hold the currently active menu and all its bitmaps.

I will commit my solution shortly.

[Vidar Hasfjord]

@jogybl wrote:

I think that the hbmpItem member will be populated if the menu item itself is a bitmap and not text.

Correct. The bitmaps we assign in Classes end up in the hbmpUnchecked member.

[Vidar Hasfjord]

I have now committed my solution [r7251].

Note that this is a preliminary implementation of menu bitmap copying and ownership, which may, and likely will, have yet undiscovered and unresolved issues. Still, it seems to work well in the Classes example, so far.

Regarding further work, consider the usage of TMenu in TFrameWindow in particular. The current code has very crude use of TMenu, effectively just taking ownership and invoking the destructor at every previous DestroyMenu call site. There should be better ways to refactor the code, I feel. For example, delegating the menu ownership to a TMenu member.

Please review and test well!

[Ognyan Chernokozhev]

I tried the new code, and it looks great. haven't noticed any problems. I also added menu bitmaps to the RichEditor example.

A small suggestion - maybe move the implementation of MapColor to TMenu, and add a flag to SetMenuItemBitmaps whether MapColor to be called internally. This way there will be no need to copy and paste the current implementation in each project that uses menu bitmaps.

[Ognyan Chernokozhev]

Also I tried to add menu bitmaps to the menu of the TRichEditView class, but could not get it to work.

[Vidar Hasfjord]

@jogybl wrote:

Also I tried to add menu bitmaps to the menu of the TRichEditView class, but could not get it to work.

What was the problem? It worked very well here. So much so that I've now committed it [r7255]. Note that I have implemented the bitmap assignment in the TRichEditView constructor, where the menu descriptor for the class is defined. I.e. it is implemented in the library, not in the example.

Please see my commit and code notes as well, as I think there is room for improvement here.

A small suggestion - maybe move the implementation of MapColor to TMenu

I added TBitmap::MapColor instead, for generality [r7253].

[Vidar Hasfjord]

I had hoped to apply the fix for this bug in the upcoming maintenance update for versions 6.44 and 7, but with the change in the menu item bitmap ownership, that seemed infeasible.

However, after some sleep, I realised that all this bitmap copying isn't really essential to fixing the issue, and that bitmap ownership actually is an orthogonal issue.

I was mislead by the name of the function that needs fixing, i.e. TMenu::DeepCopy. While this function needs to copy the menu items from source to destination, it really doesn't need to deep-copy the bitmap pixels. The problem was that it only copied the item titles, not the other data in the menu item structure. Proper copying of the menu item structure data, including bitmap handles, is sufficient to address this ticket.

(Of course, DeepCopy is not really truly a deep copy without copying the data referenced by the menu items, i.e. the bitmap data, but that has always been the case and is really a naming issue we should point out in the documentation.)

For the menu merging use-case, the suggested shallow menu item copying would be sufficient. When doing menu merging, you discard the original target menu and synthesise a new menu from the menu descriptors. Working with the same bitmap data is perfectly fine in this case, and almost always what you would want, I would think.

A scenario where a true deep copy would be wanted is harder to conjure up. But, say you wanted two menu descriptors; one for light GUI mode and one for dark mode, and you wanted to set both of them up in advance, then toggle between them when the user changes preference. In this case you may want the bitmap backgrounds mapped differently between the light and dark versions, and for that you would need different bitmaps for each of the menu descriptors.

I'll keep the true deep copy and ownership changes on the trunk for now. Although it complicates the library implementation, it simplifies ownership management for clients and hence improves usability. We'll see how it pans out over time.

For 6.44 and 7, I'll implement the shallow-copy solution. I will not address the issue in 6.30 and 6.36, as these versions are not supported for use anyway. They are only stepping stones to 6.44 or 7.

[Vidar Hasfjord]

Revision [r7260] fixes the so far unfixed overload of TMenu::DeepCopy, the more general of the two, and simplifies the implementation of the other, by making it just forward to the former. The experimental deep copying of bitmaps is guarded by a preprocessor conditional, so can be disabled easily, for merging the fix into the release branches. And, indeed, the fix has now been merged into 6.44 and 7 [r7261].

Please review and test.

Edit: A few menu bitmaps were added for testing in "branches/7/examples/classes" [r7266]. Test results: The bitmaps are preserved when switching between MDI children, so the rebuilding of the main menu from menu descriptors obviously works, copying the bitmap handles from the menu descriptor along with item titles. Looks good to go!