UI: add containing `View` for each `Label` #306

egorzhdan · 2020-12-05T15:39:22Z

WC_STATIC control doesn't receive events like WM_CONTEXTMENU. To be able to handle those events, let's create containing views for WC_STATIC instances.

compnerd · 2020-12-05T23:27:01Z

Hmm, can we not just adjust the owner window for label and handle the event?

egorzhdan · 2020-12-06T17:22:05Z

I think it would significantly complicate the Window event handling logic: we'd have to go through all the labels in all the components inside the window, find the labels that correspond to the coordinates of the mouse click, filter out the hidden ones, and find the topmost enabled label.
Similar logic would be required for other interactions to work, like drag & drop.

I believe it would be better to delegate this work to the OS, just like for all the other components.

compnerd · 2020-12-19T00:28:09Z

Thinking more about this, is this really a special case? Will this be needed for all the controls? Can we avoid this by doing a custom draw? The hit testing can be delegated to the OS, and is something I want to expose.

egorzhdan · 2020-12-21T17:26:09Z

is this really a special case

I think so, it seems WC_STATIC is the only built-in control that doesn't receive all the click events. At least I couldn't find another such control.

Will this be needed for all the controls?

Nope, this is only needed for WC_STATIC.
Actually I've just noticed that ImageView also relies on WC_STATIC, so it might need a similar workaround as Label.

Can we avoid this by doing a custom draw?

Not sure... I'll try.

The hit testing can be delegated to the OS

This would be great, this way there won't be the need to reimplement the system event dispatch logic. I don't really know how to do it though...

A similar issue will probably arise once drag-and-drop or other interactions are implemented – these static controls won't be receiving the events properly.

compnerd · 2021-03-31T00:59:06Z

This would be good to get rebased and prepared to be merged, at least for the Label case. We should figure out how to do this more generically at some point.

egorzhdan · 2021-03-31T10:16:51Z

Just rebased this, and it still works for me – the context menus show up on a right-click on labels as expected.
I haven't tried to abstract this logic away from Label in this PR, but I think it would eventually be needed to support context menus for Image.

compnerd · 2021-03-31T15:51:09Z

Hey @egorzhdan, sorry to do this to you, but could you rebase this on top of #434 as I think that is a pretty simple change and I was thinking about a bit more tweaking to this change?

Sources/SwiftWin32/Views and Controls/Label.swift

compnerd · 2021-04-03T02:48:22Z

Thinking about this a bit further, is there any advantage to actually deriving the label from View? That is, perhaps it makes more sense to just manually create the WC_STATIC window locally and add it as a child and forget about it entirely. We should be able to just handle the resize as didSet observers on the frame.

egorzhdan · 2021-04-03T18:11:45Z

If we get rid of the LabelView component, we'll have to reimplement some of View's logic, namely font & text properties, for the manually created WC_STATIC component. Do you think that would still be a better option?

compnerd · 2021-04-04T00:09:26Z

If we get rid of the LabelView component, we'll have to reimplement some of View's logic, namely font & text properties, for the manually created WC_STATIC component. Do you think that would still be a better option?

True, but, we end up having to do that in senses anyways - we forward the settings to the subview. At the end of the day, we just need to change the hWnd that the messages are sent to, so that seems no more expensive really, it just is a matter of inlining the call. If I am not mistaken, instead of forwarding the properties, we will just inline the SendMessageW for those cases right?

egorzhdan · 2021-04-05T20:12:12Z

we just need to change the hWnd that the messages are sent to

Hmm, do you mean changing self.hWnd to the WS_STATIC component's hWnd? I don't think that would work, since the parent component would get overwritten when adding the label as a subview to another view.

it just is a matter of inlining the call

So basically calling all the WinSDK methods for creating/repositioning the WS_STATIC component from Label.init?

compnerd · 2021-04-05T21:58:18Z

Hmm, do you mean changing self.hWnd to the WS_STATIC component's hWnd? I don't think that would work, since the parent component would get overwritten when adding the label as a subview to another view.

No, that won't work for exactly the reason you mention. However, rather than having the properties forward to another view, which then just invokes SendMessageW, we can inline those into the properties instead and use the stashed HWND for the static label. You counted a small set of properties which needed to be forwarded, so this should be equivalent.

So basically calling all the WinSDK methods for creating/repositioning the WS_STATIC component from Label.init?

No, the thought that I had was actually the insight that we dont care about the settings for the LabelView. The only things we care about are:

Font
Foreground Color
Position

If we set the background color to clear, instead of forwarding the font and foreground color we send the proper message to the emebdded view, and set a property observer on the position, we can entirely ignore the embedded window.

egorzhdan · 2021-04-06T16:34:03Z

Got it, thanks! I think this should now work the way you expect.

compnerd

I think that this is actually going to be far more important than I first expected. The Label type really should be a composite type which doesn't leak the members. Thank you so much for working on this!

Sources/SwiftWin32/Views and Controls/Label.swift

compnerd · 2021-04-06T17:40:48Z

Sources/SwiftWin32/Views and Controls/Label.swift

+                                      0, 0,
+                                      Int32(staticFrame.size.width),
+                                      Int32(staticFrame.size.height),
+                                      nil, nil, GetModuleHandleW(nil), nil)!


Similarly, why not use the bounds from the parent? By making the label the full size of the parent, the fact that it is embedded doesn't actually leak.

compnerd · 2021-04-06T17:43:08Z

Sources/SwiftWin32/Views and Controls/Label.swift

+    var staticStyle: LONG = GetWindowLongW(self.staticHWnd, WinSDK.GWL_STYLE)
+    staticStyle |= WS_CHILD
+    staticStyle &= ~LONG(bitPattern: WS_POPUP | WS_CAPTION)
+    _ = SetWindowLongW(self.staticHWnd, WinSDK.GWL_STYLE, staticStyle)


Is there a reason for the style to be computed? Is it not statically known?

Yeah, we actually only need WS_CHILD here, so I made it static.

Sources/SwiftWin32/Views and Controls/View.swift

`WC_STATIC` control doesn't receive events like `WM_CONTEXTMENU`. To be able to handle those events, let's create containing views for `WC_STATIC` instances. Co-authored-by: Saleem Abdulrasool <[email protected]>

compnerd

Thanks! I think that this can be simplified a bit further, but we can do that in a follow up clean up. Assuming it also builds with CMake, I don't think that we need to push this out any further.

compnerd · 2021-04-06T20:44:50Z

/azp run VS2019

azure-pipelines · 2021-04-06T20:44:58Z

Azure Pipelines successfully started running 1 pipeline(s).

compnerd · 2021-04-06T20:51:51Z

Ugh, I think looking into the CMake build can happen later.

egorzhdan mentioned this pull request Dec 5, 2020

UI: support showing basic context menus #246

Merged

compnerd changed the base branch from master to main March 26, 2021 03:55

egorzhdan force-pushed the label-hwnd branch from 3362112 to 2b49621 Compare March 31, 2021 10:16

compnerd requested changes Mar 31, 2021

View reviewed changes

egorzhdan force-pushed the label-hwnd branch 2 times, most recently from 8b27859 to 84c7495 Compare April 3, 2021 17:46

egorzhdan force-pushed the label-hwnd branch from 84c7495 to ec3ed58 Compare April 3, 2021 18:32

egorzhdan force-pushed the label-hwnd branch from ec3ed58 to 90630fd Compare April 6, 2021 16:33

egorzhdan requested a review from compnerd April 6, 2021 16:34

compnerd reviewed Apr 6, 2021

View reviewed changes

UI: add containing View for each Label

114058a

`WC_STATIC` control doesn't receive events like `WM_CONTEXTMENU`. To be able to handle those events, let's create containing views for `WC_STATIC` instances. Co-authored-by: Saleem Abdulrasool <[email protected]>

egorzhdan force-pushed the label-hwnd branch from 90630fd to 114058a Compare April 6, 2021 19:11

compnerd approved these changes Apr 6, 2021

View reviewed changes

compnerd merged commit b354fce into compnerd:main Apr 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: add containing `View` for each `Label` #306

UI: add containing `View` for each `Label` #306

egorzhdan commented Dec 5, 2020

compnerd commented Dec 5, 2020

egorzhdan commented Dec 6, 2020

compnerd commented Dec 19, 2020 •

edited

Loading

egorzhdan commented Dec 21, 2020

compnerd commented Mar 31, 2021

egorzhdan commented Mar 31, 2021

compnerd commented Mar 31, 2021

compnerd commented Apr 3, 2021

egorzhdan commented Apr 3, 2021

compnerd commented Apr 4, 2021

egorzhdan commented Apr 5, 2021

compnerd commented Apr 5, 2021

egorzhdan commented Apr 6, 2021

compnerd left a comment

compnerd Apr 6, 2021

egorzhdan Apr 6, 2021

compnerd Apr 6, 2021

egorzhdan Apr 6, 2021

compnerd left a comment

compnerd commented Apr 6, 2021

azure-pipelines bot commented Apr 6, 2021

compnerd commented Apr 6, 2021

UI: add containing View for each Label #306

UI: add containing View for each Label #306

Conversation

egorzhdan commented Dec 5, 2020

compnerd commented Dec 5, 2020

egorzhdan commented Dec 6, 2020

compnerd commented Dec 19, 2020 • edited Loading

egorzhdan commented Dec 21, 2020

compnerd commented Mar 31, 2021

egorzhdan commented Mar 31, 2021

compnerd commented Mar 31, 2021

compnerd commented Apr 3, 2021

egorzhdan commented Apr 3, 2021

compnerd commented Apr 4, 2021

egorzhdan commented Apr 5, 2021

compnerd commented Apr 5, 2021

egorzhdan commented Apr 6, 2021

compnerd left a comment

Choose a reason for hiding this comment

compnerd Apr 6, 2021

Choose a reason for hiding this comment

egorzhdan Apr 6, 2021

Choose a reason for hiding this comment

compnerd Apr 6, 2021

Choose a reason for hiding this comment

egorzhdan Apr 6, 2021

Choose a reason for hiding this comment

compnerd left a comment

Choose a reason for hiding this comment

compnerd commented Apr 6, 2021

azure-pipelines bot commented Apr 6, 2021

compnerd commented Apr 6, 2021

UI: add containing `View` for each `Label` #306

UI: add containing `View` for each `Label` #306

compnerd commented Dec 19, 2020 •

edited

Loading