search

capy-ui / capy

💻Build one codebase and get native UI on Windows, Linux and Web
https://capy-ui.org
Mozilla Public License 2.0
1.6k stars 60 forks source link

Added menubar to win32 backend #59

Closed mochalins closed 11 months ago

mochalins commented 12 months ago

Menu Bar added to win32 backend. Concerned about critical error handling (e.g. failure to allocate necessary memory) due to setMenuBar no error set interface.

image

mochalins commented 12 months ago

Edit: realized this PR was made in haste, haven't yet implemented actually reacting to events and calling callbacks.

zenith391 commented 12 months ago

Thanks you for your PR! If you add reacting to events can you also handle destroying old menus? Because currently if there's code with

window.setMenuBar(menuBar1);
window.setMenuBar(menuBar2);

the win32 menus created by the first function call will leak.

mochalins commented 11 months ago

Thanks you for your PR! If you add reacting to events can you also handle destroying old menus? Because currently if there's code with

window.setMenuBar(menuBar1);
window.setMenuBar(menuBar2);

the win32 menus created by the first function call will leak.

Leak on setting menus twice has been fixed!

Functionality-wise, menu bars can now be created/nested and events properly call user callbacks.

I do, however, hope to get your feedback on two things:

  1. Error handling on allocation failure (doesn't seem like a recoverable error and thus would like to pass it up the call stack with try, but setMenuBar has no error set in its interface)
  2. Setting classUserdata to be the Window struct's pointer for event handling - feels a little hacky, but couldn't find a nicer solution without more significant changes. Not sure if this breaks any existing functionality, e.g. if classUserdata for Window is currently being used for something else. Didn't observe any regressions in the examples I tested.

Pending your feedback on the aforementioned concerns, I think the PR appears ready for merge. @zenith391

mochalins commented 11 months ago

As an aside; the notepad example now works as intended on Windows with the latest TextArea merge.

zenith391 commented 11 months ago
  1. After some thoughts, I think you should just change setMenuBar to make it able to return errors (so change void return to !void). This means you'll have to do the same thing on the GTK backend (changing void to !void) and you'll need to update the examples accordingly. Since the only error appears to be being out of memory, using try window.setMenuBar is a good replacement.
  2. Normally, classUserdata is meant for flat components (that is components that are drawn by capy instead of the OS), however for your use case there's already peerPtr which is meant exactly for that. It's hacky (and breaks a few usecases) but it's what I came up for those times were win32 is hard
zenith391 commented 11 months ago

@mochalins Is the pull request ready to merge?

mochalins commented 11 months ago

@zenith391 hey, sorry for the lack of update! PR is ready except the error union interface for setMenuBar; I ran into strange compilation issues throughout the library when changing that for Windows+GTK backends, and I was unsuccessful in resolving or tracking down where the issues were coming from for a few hours before getting sidetracked with other work.

It's unlikely I can get back to debugging that particular issue within the next few weeks, so feel free to let me know whether you'd like me to look into it again later before the PR is merged, or whether you'd like to merge this first and deal with the error interface later.

Thanks!