naqvis / webview

Crystal bindings to Webview library
MIT License
93 stars 8 forks source link

Webview.bind: Use to_json instead of to_s, README: fix examples #22

Closed Ragmaanir closed 1 year ago

Ragmaanir commented 1 year ago

I think there is a bug in Webview.bind:

    # binds a callback function so that it will appear under the given name
    # as a global Javascript function.
    def bind(name : String, fn : JSProc)
      ctx = BindContext.new(@w, fn)
      boxed = Box.box(ctx)
      @@bindings[fn] = boxed

      LibWebView.bind(@w, name, ->(id, req, data) {
        raw = JSON.parse(String.new(req))
        cb_ctx = Box(BindContext).unbox(data)
        res = cb_ctx.cb.call(raw.as_a)
        @@bindings.delete(cb_ctx.cb)
        LibWebView.webview_return(cb_ctx.w, id, 0, res.to_s) # <- BUG: should be to_json
      }, boxed)
    end

The return value is converted using to_s instead of to_json. The examples just work because they are using integers. But if you use strings it complains about missing functions (because the strings are not quoted and therefore interpreted as names). If you return a Hash it throws errors because of the => symbol in the output generated by to_s.

This PR changes to_s to to_json.

In addition, i fixed the examples in the README. Several of them did not work because of the html not being passed to Webview.window or because the page couldn't load because of the missing data:text/html,.

naqvis commented 1 year ago

Thanks @Ragmaanir.

LibWebView.webview_return(cb_ctx.w, id, 0, res.to_s) # <- BUG: should be to_json

Yes, it should be to_json.

In addition, i fixed the examples in the README. Several of them did not work because of the html not being passed to Webview.window or because the page couldn't load because of the missing data:text/html,.

data:text is no longer supported in latest version of webview and commit https://github.com/naqvis/webview/commit/0312e71103b6c2e9c75e201bd99bd9735bbd1c76 was pushed on Dec 12, 2022 to update webview to its latest version. So what you see on README is tested and verified on Mac and Linux OS.

naqvis commented 1 year ago

@Ragmaanir Can you please update this PR by reverting changes to README and leaving changes proposed in bind method.

Ragmaanir commented 1 year ago

Hi, sorry for the delay. I changed the commit.

Regarding the README:

The examples in the README cause compilation errors:

 184 | wv = Webview.window(640, 480, Webview::SizeHints::NONE, "Hello WebView")
                    ^-----
Error: wrong number of arguments for 'Webview.window' (given 4, expected 5..6)

Overloads are:
 - Webview.window(width, height, hint, title, url, debug = false)

It looks like it is using the wrong overload. When i change the signature of Webview.window so that url is : String, then it compiles. What would you suggest as a fix?

naqvis commented 1 year ago

Hi @Ragmaanir , can you please ensure you are using the latest version? As from the error I can see, you are using older version of shard, while using the README instructions for updated version. webview bindings were updated in shard version v0.2.0, and crystal re-ordering issue was fixed in v0.2.1. So if you would like to pin to a specific version then please do that to at least 0.2.1 or you can remove the version restriction from your shard.yml, for it to always pick the latest version.

Ragmaanir commented 1 year ago

Hm, i am a little confused. I am executing the examples inside of the fork of naqvis/webview (master branch), not inside a custom project. I am also using crystal Crystal 1.7.2 [29f9ac503] (2023-01-23). What i did was: I copied the code from the example and pasted it in the src/webview.cr at the bottom. I also executed make clean and make. Then i ran crystal run src/webview.cr and got the compilation error. If i change the order of the overloads, it compiles.

If i just check out the master of naqvis/webview without my commit and do the steps above, i still get the compilation error.

naqvis commented 1 year ago

Thanks @Ragmaanir. I tried the same with Crystal 1.7.2 and it was breaking. Things might have got changed, so I've added type annotation to method signatures and it should work now. Going to fix to_json as well with this push. Please use latest release tag v0.2.3 https://github.com/naqvis/webview/releases/tag/v0.2.3