I began the week by testing the progress of the start menu in ReactOS.
You may remember this screenshot from last weekend’s report. In it, you can see the results of implementing vertical menus in a different, more documented, way. This method requires more work to be done per item, to make them all wrap into a new row, but it is supported by the Wine toolbar implementation ReactOS uses.
If you are even slightly OCD, you will have noticed that the arrows used to mark a menu as having a submenu are misaligned. This is not a bug in the code. The replacement Marlett font we have currently in ReactOS has the glyph misaligned. Below is a drawing test where I was testing the alignment flags of the text drawing functions. As you can see, all the alignment texts show up where you’d expect them, but the glyph is not vertically centered in the character space.
Aside from this minor visual glitch, you can see in the first screenshot that items are missing, and you can’t see any more submenus because they did not open. After investigating further, I traced the missing items to ReactOS returning unexpected flags in the MENUITEMINFO->fType member. I discovered that the ReactOS menu code (which is based on Wine’s) misuses the MF_POPUP flag as an indicator of the menu having a submenu attached to it, while it’s clear from the MSDN description of the MENUITEMINFO structure that the hSubMenu member already has enough information: this member is NULL if the menu does NOT have a submenu.
Although it wasn’t necessary, I wrote a new method of deciding the type of the menu items in rshell, so that I could verify that aside from this flag issue, the items were correct. That made me notice that not only the menu items were detected wrongly, but the button IDs, which the shell relies on to show the proper icons in each item, were also being given a wrong value, for all items marked as having a submenu. It turns out the two issues were related.
The ancient method of adding items to a menu involved using AppendMenu and/or InsertMenu. These two functions use the MF_POPUP flag to indicate that the ID parameter will not be an id, and instead will be repurposed as a submenu handle. In modern versions of windows (win2k and up), the InsertMenuItem function was introduced, together with some other related functions, which work with a MENUITEMINFO structure. In this structure, the MF_POPUP flag is not used anywhere, and it has separate fields for the id (wID), and the submenu handle (hSubMenu). Because of the wrong assumption that the item ID was reused as the submenu handle, the resource loading code was wrongly setting the submenu handle in the wID field.
I fixed the ReactOS menu code (including the Wine side of it), so that the functions behave as expected, both in how they store the popup information, and how they build the menus when they are read from an application’s resource data. I took the menu changes and applied them on top of the trunk code, so that they can be reviewed more formally, and possibly the result should eventually be sent to Wine. The code is at: CORE-7966
The next issue in the list was a problem where the submenus were not showing up. This turned out to be a typo in how the menus were constructed. At this point, the start menu in ROS looked more like this:
A lot better, but the looks aren’t all. Things have to work also, and they were not working as expected.
- Opening some submenus caused access violation exceptions.
- Items did not react to clicks. (with exceptions)
- The separators weren’t drawing correctly.
- Some of the items don’t have the expected icon.
- The static items (the items below the first separator in the start menu) don’t have an icon until you show the menu a second time.
I tried to fix the first issue in the list, which I knew had to be some sort of corruption, but I couldn’t trace it beyond a call to IShellFolder->GetAttributesOf(). After many hours, I left the issue aside, and focused on other issues.
I traced the clicking issue to the SHInvokeDefaultCommand function, which calls SHInvokeCommand. The latter was implemented wrongly, and Thomas Faber helped me write a more correct version. Although it now works better, for some reason the items only get executed after a submenu of the item’s menu has been shown. Clicking on any item right away will close the menu, but the application will not launch. I put that in the queue of unfixed issues, and moved on.
I continued by fixing the separator drawing so that horizontal separators would draw. The issue itself was one small typo in TOOLBAR_LayoutToolbar, although I also changed TOOLBAR_DrawItem so that separators have the same padding as button items, instead of taking up 100% of the width.
I didn’t want the corruption issue to get “cold”, so I went back to it, and while trying to explain the issue to a flatmate, I realized one small but important detail: the m_shellFolder class member was using a pointer, instead of a safe CComPtr<>. This meant that the class pointed by this field could have been destroyed at any moment, and that’s probably what happened in ReactOS. Using CComPtr<> instead of a plain pointer fixed the crashing issue. I do not know how could it not crash in Windows, but the code was definitely wrong.
With that big issue out of the way, the bug list looks closer to this:
- Clicking an item only works after opening a submenu of the item’s menu.
- The shell32 icon list does not match Windows Server 2003 SP1.
- The static button icons don’t draw on the first time the menu is displayed.
Since I couldn’t easily fix any of those issues, I took a break from the start menu, and I dedicated some time to improving my testing workflow. With that goal, I changed the effective names of our two explorers so that the older explorer became explorer_old, and the new explorer became simply explorer.exe. This way, when compiling shell-experiments, the result is a ReactOS that shows the results of those experiments right away, without having to kill a process and launch another.
Thinking of the near future, I “hackplemented” rudimentary support for explorer-new to open a browser window when a folder shortcut is launched from the desktop, such as for the “My Computer” icon.
Also, I took the chance to make browseui make use of the rshell MenuBand, and I spent all of Friday trying to get this menuband to function when running the “filebrowser.exe” program Giannis wrote.
The results are not yet functional, but some progress was made, both in Reactos:
and in Windows:
Until next weekend!