-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support emscripten build #1145
base: master
Are you sure you want to change the base?
support emscripten build #1145
Conversation
src/lib/fcitx-utils/CMakeLists.txt
Outdated
event_libuv.cpp) | ||
event_sdevent.cpp) | ||
elseif (EMSCRIPTEN) | ||
list(APPEND FCITX_UTILS_SOURCES event_js.cpp) # implemented in fcitx5-js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like having an external file dependency.
I think we could somehow, create a stub for EventLoop and having a global factory class to allow external event loop implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea how to achieve that with a factory.
Added the event_js.cpp, since there is nothing specific to fcitx5-js. emscripten_async_call
is generic API https://emscripten.org/docs/api_reference/emscripten.h.html#c.emscripten_async_call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without breaking the API, here's what you can do.
- create a stub only EventLoop that takes a EventLoopImpl as parameter.
- Create a global factory callback that allow external code to override the creation of EventLoopImpl.
- Before initialize Instance, set the factory function
In other words, use Dependency injection to make your js eventloop implementation live outside of the main repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
9152634
to
abf15e4
Compare
See fcitx-contrib/fcitx5-js#14 for downstream build. |
Upstream patch from https://github.com/fcitx-contrib/fcitx5-js