10 Nov 2014

46984

0

Post-Incrementing the C

Hello again, people.

Wow so another week has passed, and the whole weekend too. I feel a bit like I was sucked into a temporal distortion, because I can’t really tell where all these hours have gone. Heck, I almost forgot to write the report! Thankfully, the cold I spoke about last week, it didn’t complicate itself, so I have been able to work over the week.

I suppose many of you didn’t wait for the surprise and looked at the commit logs, or read the comment thread for the “spoilers”. For the rest, or if you want the details about it, here it comes.

Ever since I started working on the project, I have been getting a very clear opinion on COM, and part of that opinion, is that it sucks to work on COM from C. Since C doesn’t have its own integrated OOP features, using COM means manual reference counting, and having to manually access the virtual function tables to reach the method pointers. For the former, there isn’t anything to make it better, although for the latter, there are a set of macros that hide this virtual pointer access into a slightly more bearable syntax. But overall, while adding a new interface to an existing class, or creating a whole new class, is straightforward in C++, doing the same in C requires a large number of different steps.

Since most of the work I have been doing requires implementing new interfaces and even adding whole new classes, I was glad that most of the shell DLLs had already been converted to C++, but that wasn’t true for explorer-new itself.

Some weeks ago (a couple months or so?), I decided to spend the weekend doing a tentative C++ conversion. This was initially just a 1:1 conversion of the filenames so that they ended in .cpp, and fixing up the pseudo-OOP syntax into proper C++ classes. This all happened in my spare time, and it was never counted in the work log, because it wasn’t one of my assigned tasks. Regardless, I really wanted it to happen.

After some weekends (and a few afternoons when I was too frustrated with some other issue and I needed to clear my head), the C++ code got to a semi-stable state, and there were just a few bugs preventing it from being functional. This was around the time when the leak hunt began, so I had to set it aside because my brain couldn’t take any more debugging, let alone in my spare time.

But then when the leak hunt ended, and all the bigger blockers were gone, and we took the chance to decide what to do next, since 0.3.17 was just out the door and there was time to polish and fix some of the issues that hadn’t been considered blockers, but are still very much “wanted”, so I thought “hey, isn’t this the perfect chance?”. The C++ conversion wasn’t supposed to add any bad bugs, since all the algorithms are essentially the same, but it would open room for many improvements in code design and structure (generally called refactoring), and hopefully it would save me time fixing some of them. So, over the past weekend, I took the time to fix the remaining issues, and on Sunday afternoon, I commited the result.

And this is when the week started.

I started by cleaning up some parts of the code that were still using the C “idioms”, and I refactored them into their C++ equivalent. Most notably, I took all the code in the notification area, and the task list, both of which worked with toolbars, and created a generic class CToolbar, which would simplify managing the toolbar window, allowing for the code to be a lot more readable.

If you have used the test builds, or built the branch yourself, you may have noticed in the debug logs some very annoying log spam coming from comctl32. I decided to take a look at it since it had been making my life harder while debugging other issues, and I traced it to the TB_SETMETRICS message. This message assigns some properties of the toolbar involved in calculating the layout of the toolbar. Namely, they define the space between the content of the buttons and their border (button padding), and the space between buttons (button spacing). This function is not implemented by WINE’s comctl32, and was being called in both toolbars, repeatedly, while it was really only necessary on theme changes.

 So, I took the calls to SetMetrics, and I moved them to a better place, but instead of leaving it like that, I wondered if these calls may be related to the wrong size and position of the buttons when themes are used in ReactOS. So I started implementing the functions, trying to be the least invasive possible (since this code would have to be submitted to WINE at some point), and sadly I saw no definite improvement. When I mentioned this on IRC, Giannis replied saying I should just test what happens in windows if I omit the SetMetrics calls. And in a very large mental facepalm (which I may or may not have executed physically), I confirmed that the calls are only affecting the small gap between buttons, and the size and position of the buttons are completely unaffected. Regardless, the logspam is gone, which is still a bonus.

After this, I looked at something that I had already been pondering before during the conversion to C++. There were two instances of a strange structure with two function pointers, used from the tray window in order to display the context menus, and execute the selected action. This struct appeared to be used in exactly the same way you’d expect someone to use the IContextMenu COM interface, and in fact, the callbacks themselves were using an internal IContextMenu pointer to obtain the menu items, so I turned those two instances of the structure into classes with IContextMenu, adapted the existing code to the new function parameters, and the tray window handler to use IContextMenu instead of the old structure, and now it’s all nicer.

Finally, I turned my eyes on the old (in terms of the shell work) issue of the menubar not closing the popup menus on a second click, one of the big-but-not-quite-blocking issues that I mentioned earlier. This issue had already been attacked by me in the past, but I didn’t have much luck. The problem is I’m still not having any. After many hours tweaking the code in different ways, it never worked the way I wanted it.

On the upside, I discovered that apparently Microsoft also gave up on making the shell menubar behave exactly like the system menubar (where the system menubar closes popups on mouseup and not mousedown, but most applications, including explorer, cancel the menu on mouse down instead). This gave me the idea that maybe I can just make it so that the menubar ignores all input until after the menu is confirmed to be closed, and maybe that way I can just avoid the problem altogether. I’ll have to try that on Monday.

This is it for today, more next weekend.

Discussion: https://www.reactos.org/forum/viewtopic.php?f=2&t=13757

This blog post represents the personal opinion of the author and is not representative of the position of the ReactOS Project.