Skip to content
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

RFC: Add initial fcitx5 support #202

Merged
merged 5 commits into from
May 8, 2021
Merged

Conversation

wengxt
Copy link

@wengxt wengxt commented Feb 23, 2021

This is an initial implementation for fcitx5. I briefly tested it, and it seems to work, but I'll need some comment regarding certain piece.

  1. Is it ok to create more than on riti_context? I noticed that the underlying user dict is shared.

  2. Riti would need to be fixed due to rust c string memory leak, see Memory leak with function like riti_suggestion_get_lonely_suggestion riti#11

  3. Right now as for the configuration file, it is not shared and stored in a fcitx specific location. This is more convenient for fcitx because it will automatically generate a UI in fcitx configtool.
    图片
    图片
    (String in the image is more a place holder purpose due to no proper i18n)

  4. Missing i18n support.
    Right now this is a pure Qt project so I'm not sure if you want to go with gettext. This could be useful if you want to use gettext and to run it on windows: https://github.com/j-jorge/libintl-lite

Copy link
Member

@mominul mominul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! 🎉
I'll fix the issue OpenBangla/riti#11 soon, so you can fix the TODO's 😊

Thanks!!

src/engine/fcitx/openbangla.cpp Show resolved Hide resolved
src/engine/fcitx/openbangla.cpp Outdated Show resolved Hide resolved
@mominul
Copy link
Member

mominul commented Mar 7, 2021

Is it ok to create more than on riti_context? I noticed that the underlying user dict is shared.

I don't think it'll be. The internal state is not shared between separate contexts, so it'll create a new context every time.

I have resolved the OpenBangla/riti#11 issue and will handle OpenBangla/riti#12 next.

@wengxt
Copy link
Author

wengxt commented Mar 24, 2021

I updated the code to read the file shared with Settings.cpp.

Also change the settings to launch openbangla-gui instead.

image

Click the button here will launch openbangla-gui right now.

@mominul
Copy link
Member

mominul commented Apr 16, 2021

@wengxt I have filed a PR OpenBangla/riti#14 which will resolve OpenBangla/riti#12.

@mominul mominul added this to In progress in 3.0.0 via automation May 4, 2021
@mominul mominul changed the base branch from master to develop May 4, 2021 23:37
Fix a typo bug which was causing Auto Vowel form setting to not work.
Try to run the CI under Ubuntu 21.04 image.
Downgrade fcitx version to 5.0.5 and install fcitx development package.
@mominul
Copy link
Member

mominul commented May 6, 2021

Hi @wengxt,
I have rebased this against the develop branch and updated it to use riti's new configuration functions. Can you review it, please?
But I had to degrade the fcitx version to 5.0.5 because there is no updated version available in any Ubuntu repositories.

  • Ubuntu 21.04 has fcitx 5.0.5 version.
  • Ubuntu 20.10 & 20.04 has fcitx 0.0 (4.99.0) which might be unusable for us. (Otherwise, we could have downgraded the version more.)

Because of the version degration, we are now having errors for missing functions etc.

/__w/OpenBangla-Keyboard/OpenBangla-Keyboard/src/engine/fcitx/openbangla.cpp: In member function 'virtual void fcitx::OpenBanglaEngine::reloadConfig()':
/__w/OpenBangla-Keyboard/OpenBangla-Keyboard/src/engine/fcitx/openbangla.cpp:422:38: error: 'const class fcitx::StandardPath' has no member named 'timestamp'
  422 |   auto time = StandardPath::global().timestamp(StandardPath::Type::Config,
      |                                      ^~~~~~~~~

I think we'll have to use workarounds for the missing functions. Can you give some details about specific features (functions etc.) we're using which were not present in 5.0.5, so I could cherry-pick them from fcitx code? Are 5.0.5 and the new versions are ABI compatible?

Thanks! 😊

Don't require iBus or Fcitx libraries when building.
Build the backend when it's library is found.
Throw fatal error when both iBus & Fcitx libraries are missing.
Naming change.
@wengxt
Copy link
Author

wengxt commented May 7, 2021

the missing function just simply read the modified time of the file, the implementation is relatively simple, but non-trivial when comes to cross platform (the complexity mainly comes for bsd). I'll take a look and may be simplify it a little bit. using fstat in conjunction with existing standardpath function might bit simpler solution.

@mominul
Copy link
Member

mominul commented May 7, 2021

@wengxt I just pushed a commit that adds relevant functions as shims for the 5.0.5 version.
Can you do a throughout review, please? I'd like to merge this PR.

@wengxt
Copy link
Author

wengxt commented May 7, 2021

@mominul Yeah, I saw that. Actually I just made a change to replace the shim with using C++17's filesystem::last_write_time, that would make it simpler since fcitx5 would require C++17 anyway.

Copy link
Member

@mominul mominul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the contribution @wengxt!

@mominul mominul merged commit 3e39ff9 into OpenBangla:develop May 8, 2021
3.0.0 automation moved this from In progress to Done May 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3.0.0
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants