webui-dev / v-webui

Use any web browser as GUI, with V in the backend and modern web technologies in the frontend.
https://webui.me
MIT License
112 stars 11 forks source link

Reimplement event handler #44

Closed ttytm closed 1 year ago

ttytm commented 1 year ago

This is probably the most complicated change I'm going to suggest here. It is a breaking change. But the improvements hopefully outweigh this.

The PR has following changes that are related to each other

Result: https://github.com/ttytm/v-webui/blob/refactor/reimpl-event-handling/src/lib.v.

It might be easiest to look at the commits separately to follow the changes. It's a lot so sorry about the headache. I hope you find your way trough and please let me know if something is unclear or should be changed.

malisipi commented 1 year ago

event.@return is used in history. It was changed for native event handling system. However i can understood that is important for removing [translated]. Using old approach is better way to make the library more stable. https://github.com/webui-dev/v-webui/blob/bdb4e10e6d5a45396b6311cb8e675e928772c3e9/examples/call_v_from_js.v#L37-L39

Also this PR is resolving some unsafe behavior of library. (You can blame me for this behaviors of library :D) https://github.com/webui-dev/v-webui/blob/c7f8d03a6b009a4d0c9fc122da43ae23947ee2dc/src/lib.v#L73-L77

After all, this PR will make everything more stable and it's required for avoid breaking changes in future.

ttytm commented 1 year ago

Thank you for you review @malisipi!

It was good to add it to make the experience with with this approach! I see myself having implemented it in some form, but then realizing after some weeks that it's good the way the C library does and looking for a good moment to reverted it 🙈. I think it won't hurt us to stay true to the parent libraries approach here, on the contrary. Now, with the upcoming version and other other API related / breaking changes that are made to all wrappers it is probably a good moment for such a move.

ttytm commented 1 year ago

Also this PR is resolving some unsafe behavior of library.

I actually didn't changed this script function in this PR

pub fn (window Window) script(javascript string, timeout u64, size_buffer int) string { 
    response := &char(' '.repeat(size_buffer).str) 
    C.webui_script(window, &char(javascript.str), timeout, response, size_buffer) 
    return unsafe { response.vstring() } 
} 

But I think we can upgrade it to make it safer and include all features. I submitted something here: #49

malisipi commented 1 year ago

Oh, i guess i misread the diff file. (Nevermind)

I haven't enough knowlenge about C/V interopability to implement this better. So I wrote this code even I know this is not safe. However refactoring this code is really good. And I thank you for it.

ttytm commented 1 year ago

Thank you @malisipi. Your code is good. Everything else now builds on that!