The MASM Forum

In an attempt to adjust the color of a simple graphics static box intended to display a filled rectangle under a small cluster of controls, I replaced the style SS_WHITERECT that I defined initially with SS_OWNERDRAW and implemented the required WM_DRAWITEM handler, but there's something really wrong, and I just can't tell what it is.

The program in question is a modeless dialog box that displays one edit control, three regular buttons, and the filled rectangle I already mentioned. When run, the program appears to work okay but, as soon as I press Alt, the rectangle is brought to the top of the z-order, which completely hides its siblings. Interestingly, even when they're hidden, the three (standard) buttons are still clickable, and it is possible to get them back by pressing Tab or simply clicking them. However, the edit control is not fully redrawn until the window is first minimized and then restored, after which the program seems to work just perfectly (that is, after the window is restored, pressing Alt again has no effect). But that's not all.

First of all, the bug manifests itself only when I start the program by running the executable file manually—when I run it in Visual Studio (either normally or with debugging) or via Command Prompt, nothing ever happens. Using IDA, I found out that every time I press the key, the DRAWITEMSTRUCT parameter of WM_DRAWITEM is "05 00 00 00 E9 03 00 00 28 01 00 00 01 00 00 00 00 00 00 00 F6 06 0C 00 54 02 01 C0 00 00 00 00 00 00 00 00 86 01 00 00 55 00 00 00 00 00 00 00", while initially it is "05 00 00 00 E9 03 00 00 00 00 00 00 01 00 00 00 00 01 00 00 F6 06 0C 00 53 0F 01 26 00 00 00 00 00 00 00 00 86 01 00 00 55 00 00 00 00 00 00 00". That is, when the message is caught, the itemID and itemState fields are 128h and 0 (respectively), while initially the values are 0 and 100h. It doesn't make any sense to me, however, because I don't have a menu, and the value itemState=0 isn't even listed in WinUser.h. The only noteworthy thing is that the initial value itemState=100h means that the control is to be drawn without the keyboard accelerator cues, which brings us to accelerators.

The problem can be 'fixed' by going to Control Panel > Ease of Access Center > Make the keyboard easier to use and ticking the checkbox Underline keyboard shortcuts and access keys. I don't know if "access key" is different from "accelerator", but I think that this may also be related as I have three virtual-key characters associated with the buttons, e.g., "N", 1003, VIRTKEY, where 1003 is the identifier of the button New. Is it correct to define them like this? Or maybe, is it possible to define a real accelerator without using the ampersand character and still have the first letter underlined?

Also, the program exampl03\menudemo\menudemo.asm, which seems to be the only example program that makes use of owner-drawn controls in masm\examples, ends its WM_DRAWITEM handler with mov dis.itemState, ODS_DEFAULT\mov dis.hdc, 0. Is it necessary?

There are three somewhat related questions on Stack Overflow but, unfortunately, none comes with a satisfying answer. I'm listing them below. Could you please help me?

________________________________________________________________________________

1. Ownerdraw controls overlaps when main window is minimized (https://stackoverflow.com/questions/77791667/ownerdraw-controls-overlaps-when-main-window-is-minimized)
2. Strange behaviour when customdrawing a button (https://stackoverflow.com/questions/42602145/strange-behaviour-when-customdrawing-a-button)
3. How to filter Alt key event for WM_DRAWITEM message (https://stackoverflow.com/questions/39693843/how-to-filter-alt-key-event-for-wm-drawitem-message)

I just took a cursory look at your code. Nothing pertaining to your problem jumps out at me yet. However, some suggestions:

• Please learn to use INVOKE instead of push-push-call: it makes your code a hell of a lot easier to read:
  
  Code Select Expand
      INVOKE    CreateDialogParamA, HINST, OFFSET DIALOGNAME, 0, OFFSET DLGPROC, 0
; versus
    PUSH  0
    PUSH  OFFSET DLGPROC
    PUSH  0
    PUSH  OFFSET DIALOGNAME
    PUSH  HINST
    CALL  CreateDialogParamA@20
(notice that you can lose the A@20 business since you're using a prototype)
  
• Don't use the old-school SEGMENTs; that's so 16-bit DOS. Just use the new simplified ones: .data, .data? and .code.
• Lose the message loop; you don't need it here, as the dialog manager establishes its own loop. Dunno if this is causing any problems, but it definitely doesn't help.
• Please don't use those horrible EBP+xxx constructs for proc parms. Define the procedure properly and you can use named variables (what a concept!). It'll make your code less error-prone and easier to maintain.
  
  Code Select Expand
  DLGPROC PROC hWin:HWIN, uMsg:DWORD, wParam:DWORD, lParam:DWORD
; instead of
;   [EBP+14H]: LPARAM -- Additional message-specific information
;   [EBP+10H]: WPARAM -- Additional message-specific information
;   [EBP+0CH]: MES -- The message
;   [EBP+08H]: HWND -- A handle to the dialog box


Sorry if I seem like a scold, but the easier you make your code for us to read, the more likely someone here can help you.

I'll look at your specific problem shortly. I should say you deserve credit for clearly documenting your problem here.

Static don't work like container.
You can try to replace Static with GroupBox.

I believe its to do with the tab index order, or the order in which the controls are created via the dialog manager that is causing the static to appear on top when the alt key is pressed. I recall vaguely something I read somewhere about the Z order not being what its supposed to be or something. Anyhow long story short is if you re-arrange the order the controls are listed in the rc file to:

DIAL1 DIALOG 0, 0, 390, 290
FONT 8, "MS Sans Serif"
STYLE WS_CAPTION | WS_SYSMENU | WS_MINIMIZEBOX | WS_VISIBLE
{


    CONTROL "", 1002, "edit",
            ES_CENTER | ES_READONLY | WS_BORDER | WS_CHILD | WS_TABSTOP | WS_VISIBLE,
                    75, 151, 240, 12

    CONTROL "&New", 1003, "button",
            BS_CENTER | BS_PUSHBUTTON | WS_CHILD | WS_TABSTOP | WS_VISIBLE,
                    125, 171, 40, 12

    CONTROL "&Copy", 1004, "button",
            BS_CENTER | BS_PUSHBUTTON | WS_CHILD | WS_TABSTOP | WS_VISIBLE,
                    175, 171, 40, 12

    CONTROL "E&xit", 1005, "button",
            BS_CENTER | BS_PUSHBUTTON | WS_CHILD | WS_TABSTOP | WS_VISIBLE,
                    225, 171, 40, 12

    CONTROL "", 1001, "static",
            SS_OWNERDRAW | WS_CHILD | WS_CLIPSIBLINGS | WS_VISIBLE,
                    65, 141, 260, 52

}

I added the & to the text in the controls, so that the accelerator is underlined when the ALT key is pressed.

Doesn't seem to be an issue with declaring the accelerator the same id as the button. For my own use and as a standard I like to keep the defines for suchlike unique, but that requires additional checking in the WM_COMMAND for them as well. Maybe some edge case issues could crop up, but cant think of anything at the moment.

Also prob no harm adding in WS_EX_TRANSPARENT to the static so that mouse clicks are ignored. But I tested and it seems to work without it, so no worries either way. Add the define to your rc:

// Extended Window Styles
#define WS_EX_TRANSPARENT 0x00002000L
and then include the extended style like so:

    CONTROL "", 1001, "static",
            SS_OWNERDRAW | WS_CHILD | WS_CLIPSIBLINGS | WS_VISIBLE,
                    65, 141, 260, 52,  WS_EX_TRANSPARENT


Quote from: HSE on February 06, 2024, 07:50:29 AMStatic don't work like container.

I'm not so sure of that. I've successfully used static controls as containers, but there are issues to be dealt with.

I'm wondering if the OP needs to position the window on the Z-axis so things layer as he wants them to (using SetWindowPos()).

Looks like your program is some kind of KeyGen.
I hope this is not for illegal purposes.

; Ensure that the device context identified by HDC is in the default state and return
    MOV  PDIS.ITEMSTATE, 20h  ; ODS_DEFAULT    ; not necessary or even maybe wrong, don't touch it !
    MOV  PDIS.HDC, 0                           ; This is wrong ! (See explanation below)

"Ensure that the device context identified by HDC is in the default state" means that you must not leave any GDI object that you selected into the HDC in it.
So, always save the current object returned by SelectObject and after use select it back into the HDC.

EDIT:
Just saw that you use a copy of the DRAWITEMSTRUCT. This si not necessary.
You can use [ESI].DRAWITEMSTRUCT.HDC, etc. ...

Greenhorn—the MOV PDIS.ITEMSTATE, ODS_DEFAULT\\MOV PDIS.HDC, 0 fragment is copied from exampl03\menudemo\menudemo.asm.

And addressing all of you, thank you for all the suggestions. I may not be able to try them out until Wednesday, but I'll definitely get back to you as soon as possible.

fearless—when declaring the accelerators, I reused the IDs of the buttons precisely because I wanted to avoid those additional checks, but otherwise you are right. Just reordering the controls so that the static one comes last fixes the problem completely.

My only follow-up question would be if it's possible for the Alt key to show and hide the accelerator cues alternately, just as in Visual Studio or Word 2016.

Not sure, but probably have to look at something to do with the following:

• WM_UPDATEUISTATE (https://learn.microsoft.com/en-us/windows/win32/menurc/wm-updateuistate)
• WM_CHANGEUISTATE (https://learn.microsoft.com/en-us/windows/win32/menurc/wm-changeuistate)
• Untangling the confusingly-named WM_UPDATEUISTATE and WM_CHANGEUISTATE messages (https://devblogs.microsoft.com/oldnewthing/20130516-00/?p=4343)
• Who sends the initial WM_UPDATEUISTATE message? (https://devblogs.microsoft.com/oldnewthing/20130517-00/?p=4323)

Quote from: Greenhorn on February 06, 2024, 08:53:31 AMJust saw that you use a copy of the DRAWITEMSTRUCT. This si not necessary.
You can use [ESI].DRAWITEMSTRUCT.HDC, etc. ...

In masm\examples\exampl03\lcd\lcd.asm, there is the following function definition. Can I also use assume? Is there any real difference?

GetPositionFromString proc uses esi edi  lpString:DWORD,lpPosition:DWORD
; Function to convert TMSF string to CDPOSITION structure.
mov edi, lpPosition
assume edi:ptr CDPOSITION
mov esi, lpString
invoke GetNextNumberFromTime
mov [edi].Track, al
invoke GetNextNumberFromTime
mov [edi].Minute, al
invoke GetNextNumberFromTime
mov [edi].Second, al
invoke GetNextNumberFromTime
mov [edi].Frame, al
ret
GetPositionFromString endp


Those ASSUMEs are completely superfluous. Get rid of them.

As long as esi & edi are pointing towards the appropriate places, that's all that matters.

In other words, the caller of GetPositionFromString() can be assumed (by us humans, not the assembler) to have passed the correct addresses in those two parameters, lpString and lpPosition. The subroutine simply obeys orders based on those addresses.

In other other words, if those parameters were somehow passed incorrectly, no ASSUME is going to fix that.

---

Ack. I'm wrong here. Forgot you were dealing with structure members. Nevermind ...

So all this just to avoid typing
[EDI].STRUCTNAME.member
Me, I just copy and paste. That way it's obvious what's being dealt with, instead of masking it behind a macro or EQU. But that's just me ...

Quote from: kkurkiewicz on February 08, 2024, 05:10:30 AMCan I also use assume?

You can, certainly. But there are more elegant ways to do that.

    cdp equ [edi.CDPOSITION]
    mov edi, lpPosition
    mov cdp.Track, al
    mov cdp.Minute, al

Or how 'bout not being lazy and typing
[edi].CDPOSITION.Track
Is that really so hard? Are we that spoiled?
Besides, to me it's much more obvious when revisiting the code a year later and trying to figure out what the hell you were trying to do ...

How would you rewrite PUSH  OFFSET PDIS.RCITEM?

So PDIS is a global** structure, correct? And you want to push the address of the RCITEM member, yes?

I wouldn't rewrite it. Why would you want to? What's wrong with it as it is?

** We can tell it's a global because of OFFSET. If it were a LOCAL we'd have to use ADDR instead.

If ASSUME is used, PDIS becomes redundant. How do I take the address of RCITEM if the only reference to DRAWITEMSTRUCT is a pointer stored in ESI?

Quote from: kkurkiewicz on February 08, 2024, 06:14:54 AMHow would you rewrite PUSH  OFFSET PDIS.RCITEM?

push offset pdis.RCITEM (in case you want to pass on the address of the RECT structure)

What I meant was that if the PDIS var is deleted, then PUSH OFFSET [ESI].RCITEM is a syntax error. How do I take the address of the RCITEM member using only ESI?

WMDRAWITEM:
; Message was not sent by the rectangle static control => return FALSE
    CMP    DWORD PTR [EBP+10H], 1001
    JNE    FINISH
; Load the DRAWITEMSTRUCT pointer into ESI and cast the register to a pointer
    MOV    ESI, DWORD PTR [EBP+14H]
    ASSUME ESI:PTR DRAWITEMSTRUCT
; Fill the rectangle with white
    PUSH   HBRUSHSTATIC
    PUSH   OFFSET [ESI].RCITEM
    PUSH   [ESI].HDC
    CALL   FillRect@12
; Ensure that the device context identified by HDC is in the default state, cancel all assumptions and return
    MOV    [ESI].ITEMSTATE, 20h  ; ODS_DEFAULT
    MOV    [ESI].HDC, 0
    ASSUME ESI:NOTHING
    MOV    EAX, 1
    JMP    FIN


Quote from: kkurkiewicz on February 08, 2024, 06:50:02 AMIf ASSUME is used, PDIS becomes redundant. How do I take the address of RCITEM if the only reference to DRAWITEMSTRUCT is a pointer stored in ESI?

Easy peasy:
[ESI].DRAWITEMSTRUCT.<member>
You just use the name of the structure here, which tells the assembler the type of the reference.

But I need a pointer...
Pardon the comparison, but it would be &(esi->rcItem) in C.

Quote from: kkurkiewicz on February 08, 2024, 07:24:58 AM; Fill the rectangle with white
    PUSH   HBRUSHSTATIC
    PUSH   OFFSET [ESI].RCITEM
    PUSH   [ESI].HDC
    CALL   FillRect@12

The proper way to do that is to use invoke. Two options:
ASSUME ESI:PTR DRAWITEMSTRUCT
invoke FillRect, [esi].hDC, addr [esi].rcItem, hWhiteBrush
ASSUME ESI:nothingdis equ [esi.DRAWITEMSTRUCT]
invoke FillRect, dis.hDC, addr dis.rcItem, hWhiteBrush
Note that the second option is much more readable, and you don't need to de-assign anything.

The push-push-call orgies look so professional, but after a while you'll just find out that they are error-prone and useless. There is a reason why invoke was created.

Note also that Masm and its clones are mostly case-insensitive. Writing a Million times PUSH will lead to Repetitive Strain Injury (https://www.nhs.uk/conditions/repetitive-strain-injury-rsi).

Quote from: kkurkiewicz on February 08, 2024, 07:46:06 AMBut I need a pointer...
Pardon the comparison, but it would be &(esi->rcItem) in C.

In that case you need to add ADDR to the equation:

&(esi->rcItem) <--> ADDR [ESI].DRAWITEMSTRUCT.rcItem

(What goes on "behind the scenes" is this:
LEA   EAX, [ESI].DRAWITEMSTRUCT.rcItem
leaving EAX with the address of that member of the structure, which is just the value of ESI plus the offset of that member)
(Which is why you must always be aware of this when using ADDR, and be prepared for EAX to be trashed)

Quote from: NoCforMe on February 08, 2024, 09:53:08 AMwhen using ADDR .. be prepared for EAX to be trashed

Don't worry: if that is the case, invoke will throw an error.

Yes. BTW, I totally endorse JJ's advice to use INVOKE.

Quote from: kkurkiewicz on February 08, 2024, 07:24:58 AMWhat I meant was that if the PDIS var is deleted, then PUSH OFFSET [ESI].RCITEM is a syntax error. How do I take the address of the RCITEM member using only ESI?

WMDRAWITEM:
    ...
    ASSUME ESI:PTR DRAWITEMSTRUCT
; Fill the rectangle with white
    PUSH   HBRUSHSTATIC
    LEA    EAX, [ESI].RCITEM
    PUSH   EAX
    PUSH   [ESI].HDC
    CALL   FillRect@12
    ...

Thank you :thumbsup:
Next time I will use INVOKE, I promise.

Quote from: kkurkiewicz on February 08, 2024, 07:44:57 PMNext time I will use INVOKE, I promise.

include \masm32\include\masm32rt.inc    ; has all the definitions, PROTOs, etc

.data?
hWhiteBrush    dd ?
_dis    DRAWITEMSTRUCT <>

.code
start:
  ; option A, shorter code:
  mov esi, offset _dis
  dis equ <[esi.DRAWITEMSTRUCT]>
  invoke FillRect, dis.hdc, addr dis.rcItem, hWhiteBrush

  ; option B, more code under the hood (4 bytes more):
  invoke FillRect, _dis.hdc, addr _dis.rcItem, hWhiteBrush
end start
(just to demonstrate the techniques - code will build but crash, since there is no DC, brush etc yet)

Have a look at \masm32\include\masm32rt.inc, it contains all the definitions that you collected in proc.inc (you wouldn't re-write WinUser.h if it was C++, right?)

If you write...
invoke FillRect, dis.hdc, addr dis.rcItem, hWhiteBrush, eax
... the assembler will shout at you: Too many arguments to INVOKE

It will not shout if you do it with push-push-call. No problem, of course, you can count to three, right? But there are functions like CreateFont with 14 arguments, or CreateWindowEx with 11; or try NtAccessCheckByTypeResultListAndAuditAlarmByHandle (https://microsoft.github.io/windows-docs-rs/doc/windows/Wdk/Storage/FileSystem/fn.NtAccessCheckByTypeResultListAndAuditAlarmByHandle.html) (defined in \Masm32\include\ntdll.inc). It's easy to get lost with push-push-call, your stack will be 4 bytes off, and such bugs are very difficult to chase. Use invoke.