UI: support showing basic context menus #246

egorzhdan · 2020-10-30T17:18:09Z

This change adds support for basic context menus (attached to a View instance) and window title menus. For both types of menus submenus are supported.

Sources/UI/MenuElement.swift

-  public var image: Image?
+  public let image: Image?
+
+  public typealias Submenu = ContextMenu


egorzhdan · 2020-10-30T17:20:58Z

I’ve changed the submenu structure a little bit: moved title from Menu to MenuItem for a couple reasons:

context menus don’t have a title
window title menu items are not required to have submenus, they can just act as buttons

@compnerd please let me know what you think about this!

compnerd · 2020-11-05T16:08:31Z

discussed offline - the API surface is likely going to revert to something that was there previously

egorzhdan · 2020-11-21T20:18:25Z

(only rebased for now, will address the feedback a bit later)

Sources/SwiftWin32/UI/MenuElement.swift

compnerd · 2020-11-28T21:46:14Z

Examples/UICatalog/UICatalog.swift

@@ -110,6 +110,19 @@ final class UICatalog: ApplicationDelegate, SceneDelegate {
    window.addSubview(self.tableview)
    window.addSubview(self.imageview)

+    window.rootViewController?.titleMenu = Menu(children: [
+      Menu(title: "Foo", children: [


I'm not a fan of Foo and Bar. What do you think of creating a UICatalog menu (or perhaps Application?) with an entry for Quit?

I think it would be nice to have a mixture of predefined menu items and custom menu items.

Yeap, sounds good, replaced the menu with "Application", since it's right under the window title which says "UICatalog".

If I understand the API design correctly, the logic for suggested menu items would belong somewhere in ContextMenuConfiguration, but I haven't really touched it yet.

Actually, I realized that you are looking at context menus, not application menus. In such a case, perhaps the application doesn't make sense :-(. But creating a new UI element to attach the context menu makes sense (perhaps a TextField?)

Oh, I've mixed up the menu types in my comment above, sorry. The titleMenu field is about window title menus, not context menus, so ContextMenuConfiguration doesn't apply here.

compnerd · 2020-11-28T21:47:27Z

Examples/UICatalog/UICatalog.swift

+      ]),
+    ])
+
+    self.progress.addInteraction(ContextMenuInteraction(delegate: self))


What do you think of adding an additional label to demonstrate the context menu? I'd like to eventually migrate to a more fleshed out application with a view controller per type of control and demonstrate different behaviors.

Turns out the menu logic doesn't actually work for Labels – the WM_CONTEXTMENU events are dispatched to a Window containing the label instead of the label itself.
We'll probably need a LabelProxy to be able to handle the events properly, I'll add it in a separate PR.

Adding the proxy sounds good to me, and as a separate PR sounds perfect!

Unfortunately adding the proxy didn't help, but wrapping the label in a view solves the issue. Sent #306.

Sources/SwiftWin32/UI/Interaction.swift

Sources/SwiftWin32/UI/Menu.swift

Sources/SwiftWin32/UI/MenuElement.swift

compnerd · 2020-11-29T18:31:26Z

Sources/SwiftWin32/UI/MenuElement.swift

@@ -67,3 +69,48 @@ public class MenuElement {
    self.image = image
  }
 }
+
+internal class Win32MenuElement {


Are there reference semantics that are needed, or can we use value semantics here?

We're passing a reference to Win32MenuElement.info as a parameter to InsertMenuItemW on Menu.swift:303, that won't compile if Win32MenuElement is a struct.

Menu.swift:303:23: error: cannot pass immutable value as inout argument: 'child' is a 'let' constant &(child.info)) ^ ~~~~~

Hmm, what if you make children a var instead? I think that it might be possible to get away with that.

Nope, that doesn't help, the same error occurs. That's probably because even though children is mutable, its elements still need mutating keyword for modifying operations.

compnerd · 2020-12-03T03:52:43Z

Sources/SwiftWin32/UI/MenuElement.swift

+    } else {
+      subMenu = nil


This is unnecessary, the declaration should be initialized to nil anyways.

If I remove the else block, it stops compiling:

swift-win32\Sources\SwiftWin32\UI\MenuElement.swift:105:60: error: constant 'subMenu' used before being initialized return Win32MenuElement(title: element.title, subMenu: subMenu, fType: MFT_STRING) ^

compnerd · 2020-12-03T03:54:49Z

Sources/SwiftWin32/UI/View.swift

+    } else {
+      view.win32ContextMenu = nil


This should not be need right?

This is executed if the menu was non-nil during previous contextMenuInteraction invocation, but is nil now and shouldn't be shown. The assignment also removes the reference to the outdated menu, allowing it to deinit if not referenced anywhere else.

compnerd · 2020-12-03T03:55:30Z

/azp run VS2019

azure-pipelines · 2020-12-03T03:55:37Z

Azure Pipelines successfully started running 1 pipeline(s).

egorzhdan · 2020-12-08T11:05:33Z

I think I've addressed all the problematic parts: removed window title menus (will add as a separate PR), and fixed the code issues.

compnerd · 2020-12-08T16:46:40Z

/azp run VS2019

azure-pipelines · 2020-12-08T16:46:48Z

Azure Pipelines successfully started running 1 pipeline(s).

compnerd

Thanks!

egorzhdan · 2020-12-08T17:35:54Z

Just added the missing file to CMakeLists, the build should pass now.

egorzhdan commented Oct 30, 2020

View reviewed changes

Sources/UI/MenuElement.swift Outdated

public var image: Image?

public let image: Image?

public typealias Submenu = ContextMenu

This comment was marked as outdated.

Sign in to view

egorzhdan marked this pull request as draft November 21, 2020 20:15

egorzhdan force-pushed the master branch from 449a5f1 to d1831f8 Compare November 21, 2020 20:17

egorzhdan force-pushed the master branch 4 times, most recently from ea1bcb9 to c5bf2f7 Compare November 27, 2020 19:52

compnerd reviewed Nov 28, 2020

View reviewed changes

Sources/SwiftWin32/UI/MenuElement.swift Show resolved Hide resolved

egorzhdan mentioned this pull request Nov 28, 2020

UI: tweak MenuElement public interface #294

Merged

egorzhdan force-pushed the master branch from c5bf2f7 to c8b7345 Compare November 28, 2020 20:36

compnerd reviewed Nov 28, 2020

View reviewed changes

egorzhdan force-pushed the master branch from c8b7345 to af570e6 Compare November 29, 2020 11:42

compnerd reviewed Nov 29, 2020

View reviewed changes

egorzhdan force-pushed the master branch 4 times, most recently from f4484c4 to 68b0a98 Compare December 1, 2020 10:12

compnerd reviewed Dec 3, 2020

View reviewed changes

egorzhdan force-pushed the master branch 2 times, most recently from 347818f to a24d8da Compare December 8, 2020 11:00

egorzhdan marked this pull request as ready for review December 8, 2020 11:05

egorzhdan changed the title ~~UI: support showing basic context & window menus~~ UI: support showing basic context menus Dec 8, 2020

compnerd approved these changes Dec 8, 2020

View reviewed changes

egorzhdan force-pushed the master branch from a24d8da to ffe88cd Compare December 8, 2020 17:34

UI: support showing basic context menus

2cffd9a

egorzhdan force-pushed the master branch from ffe88cd to 2cffd9a Compare December 8, 2020 17:45

compnerd merged commit 842d5c3 into compnerd:master Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: support showing basic context menus #246

UI: support showing basic context menus #246

egorzhdan commented Oct 30, 2020

This comment was marked as outdated.

egorzhdan commented Oct 30, 2020

compnerd commented Nov 5, 2020

egorzhdan commented Nov 21, 2020

compnerd Nov 28, 2020

egorzhdan Nov 29, 2020

compnerd Nov 29, 2020

egorzhdan Dec 5, 2020

compnerd Nov 28, 2020

egorzhdan Nov 29, 2020

compnerd Nov 29, 2020

egorzhdan Dec 5, 2020

compnerd Nov 29, 2020

egorzhdan Dec 1, 2020

compnerd Dec 1, 2020

egorzhdan Dec 2, 2020

compnerd Dec 3, 2020

egorzhdan Dec 3, 2020

compnerd Dec 3, 2020

egorzhdan Dec 3, 2020

compnerd commented Dec 3, 2020

azure-pipelines bot commented Dec 3, 2020

egorzhdan commented Dec 8, 2020

compnerd commented Dec 8, 2020

azure-pipelines bot commented Dec 8, 2020

compnerd left a comment

egorzhdan commented Dec 8, 2020

UI: support showing basic context menus #246

UI: support showing basic context menus #246

Conversation

egorzhdan commented Oct 30, 2020

This comment was marked as outdated.

egorzhdan commented Oct 30, 2020

compnerd commented Nov 5, 2020

egorzhdan commented Nov 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compnerd commented Dec 3, 2020

azure-pipelines bot commented Dec 3, 2020

egorzhdan commented Dec 8, 2020

compnerd commented Dec 8, 2020

azure-pipelines bot commented Dec 8, 2020

compnerd left a comment

Choose a reason for hiding this comment

egorzhdan commented Dec 8, 2020