MenuItemOptions click property incorrect

[Shmew]

The MenuItemsOptions click property is incorrect.

The property has a function signature of: MenuItem -> BrowserWindow -> KeyboardEvent -> unit when it should be unit -> unit

I was able to work around this by doing:

let mutable tray: Tray option = Option.None

[<Emit("$0")>]
let menuItemOptionsPojo menuItemOptions : MenuItemOptions = jsNative

let show =
    menuItemOptionsPojo
        ({| label = "Open"
            click = 
                fun () -> 
                    match mainWindow with
                    | Some(win) -> win.show()
                    | None -> () |}) 
    |> U2.Case1

let appTray =
    main.Tray.Create("")

main.Menu.buildFromTemplate [| show |]
|> Some
|> appTray.setContextMenu

tray <- Some appTray

[cmeeren]

Are you sure? The current signature is according to the Electron docs.

[Shmew]

Yeah, that had me confused as well.

I looked around a lot on the web and never saw anyone set the click property to anything but unit -> unit. I just found one blog post published recently that used that function signature, but I never had any success with it. I usually did it something like:

let show =
    ({| label = "Open"
            click = 
                fun (_, _, _) -> 
                    match mainWindow with
                    | Some(win) -> win.show()
                    | None -> () |}
    |> U2.Case1

Which would never fire. At the very least I can confirm that it does accept unit -> unit. You can also see them do this in the official docs here.

[cmeeren]

Yes, AFAIK in JS you can drop arguments if you don't need them, so even if you have a callback signature like a -> b -> unit you can supply unit -> unit.

Come to think of it though... I had a similar issue recently where I explicitly had to wrap the callback in Func: https://github.com/Zaid-Ajaj/Feliz/issues/27

Could you try that with your callback? I.e., something like:

let show =
    ({| label = "Open"
            click = 
                Func<_,_,_,_>(fun (_, _, _) -> 
                    match mainWindow with
                    | Some(win) -> win.show()
                    | None -> ()) |}
    |> U2.Case1

[Shmew]

Yeah it doesn't like that since it's expecting three arguments it treats the System.Func as a single argument of the three required. Unless I'm just doing it wrong. My repo is public if you want to look at it yourself.

[cmeeren]

Oh, right. For the moment, just for testing, try "cheating" by unboxing the Func to the correct type: unbox(fun (_, ...

[Shmew]

I was able to get this to work:

let show =
    main.MenuItem.Create
        (jsOptions<MenuItemOptions> (fun o ->
            o.label <- "Open"
            o.click <- 
                unbox(System.Func<_,unit>(fun (_, _, _) ->
                    match mainWindow with
                    | Some(win) -> win.show()
                    | None -> ()
                )))
            )
    |> U2.Case2

[cmeeren]

Dang. That likely means that all multi-parameter callback properties in Fable.Electron suffer from the same problem as in Zaid-Ajaj/Feliz#27.

[cmeeren]

@Zaid-Ajaj or @alfonsogarciacaro do you have any tips on how to write bindings like the below, considering that the code below doesn't work due to currying?

https://github.com/fable-compiler/fable-electron/blob/bcffb111efc02688d936a0c7b2ea80137ef2ba74/src/Fable.Electron/Bindings.fs#L7381

[MangelMaxime]

Seems like the "direct" solution is to create an alias U3<Func<MenuItem, BrowserWindow, KeyBoardEvent, unit>, Func<MenuItem, BrowserWindow, unit>, Func<MenuItem, unit>.

I know it looks harder to read but should solve the problem. Other solutions probably need something from the compiler itself I guess.

[cmeeren]

Will using Func force the user to use Func explicitly too, or is that handled automatically by F#/Fable? (I know it is in some cases, but not all, and I don't know exactly which.)

Regarding U3: Given how hard the signature is to read, and the fact that the user must use U3.CaseX or !^, I think it's better to only have the most general callback, and users can just use _ for the ignored parameters.

[Zaid-Ajaj]

When you use jsOptions you limit yourself in how you initialize the properties and you cannot do "postprocessing" internally in the library to hide some ugly js parts. I guess the best way here is to use overloaded click handlers and then cast them to Func internally before making the property.

I haven't tested the code below but it goes something like this

type IMenuItemProperty = interface end 

type MenuItem with 
    static member click (handler: MenuItem -> BrowserWindow -> KeyboardEvent -> unit) : IMenuItemProperty  = 
      !!("click", System.Func<_, _, _, _> handler)
    static member click (handler: MenuItem -> BrowserWindow -> unit) : IMenuItemProperty  = 
      !!("click", System.Func<_, _, _> handler)
    static member create (props: IMenuItemProperty list) = main.MenuItem.Create (!!createObj (!!props))

main.MenuItem.create [
   MenuItem.label "show"
   MenuItem.click (fun item window keyboardEvent -> ())
   MenuItem.click (fun item window -> ())
   MenuItem.click (fun item -> ())
] 

Of course this property list syntax doesn't always make sense but in case of UI elements (menu items in this case) it would work nicely

[cmeeren]

Thanks! Yes, the Electron bindings are currently "very JS".

General note: Suggestions for improvements, and PRs, are welcome! I have very limited capacity to work on significant changes to this library. Therefore, at the moment, I'm just looking for a quick fix to actually make this work (and for similar other multi-parameter function properties).

[MangelMaxime]

Therefore, at the moment, I'm just looking for a quick fix to actually make this work

To test but then perhaps:

abstract click: Func<MenuItem, BrowserWindow, KeyboardEvent, unit> with get, set

Force the user to provide a Func<> construct.

[cmeeren]

Come to think of it, would that just be Action<MenuItem, BrowserWindow, KeyboardEvent>?

[cmeeren]

Side note regarding the bindings being a thin JS wrapper: That's not necessarily (just) a bad thing. Electron has a large and complicated API, and keeping the bindings close to JS allows people to use the official docs and not have to learn a different, custom API.

And while API ergonomics ideally trumps maintenance, a completely F#-idiomatic API might be a massive undertaking both to implement and document. And when the maintainer is stripped for time, a thin JS wrapper API may be the best choice, since it's more likely that the bindings will be kept up to date.

Just some thoughts.

(Of course, improved syntaxes could be designed for parts of the API, e.g. creating MenuItems as described by @Zaid-Ajaj in https://github.com/fable-compiler/fable-electron/issues/4#issuecomment-530299789), which could use the JS-wrapping API.

[cmeeren]

@Shmew 3.0.0 will be published soon. Please let me know if it's fixed.

[MangelMaxime]

I agree with you @cmeeren my position in general is to have a pure bindings.

And if people needs something more high level, we (or the consumer) can either have an Helpers module or a dedicated package for that.