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

Remove text prediction feature for now #146

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Conversation

dobey
Copy link
Contributor

@dobey dobey commented Jul 12, 2022

As presage has been unmaintained since 2018, among various other concerns with
the feature and how the ngram databases are created, remove text prediction for
now until a better implementation can be done, after keyboard layouts support
has been refactored to be much simpler.

@dobey
Copy link
Contributor Author

dobey commented Jul 26, 2022

Also, maybe @sunweaver might have an opinion on this, as he's maintaining the packages in Debian for maliit now. Would be good for some feedback there.

@sunweaver
Copy link
Contributor

As we want to replace lomiri-keyboard by maliit-keyboard in Ubuntu Touch 22.04, this feature will probably dearly be missed. For Debian, it is not so relevant, because people will never have used the presage feature in a Debian stable release.

Also, presage is in maintenance-only mode in Debian, as well: https://metadata.ftp-master.debian.org/changelogs//main/p/presage/presage_0.9.1-2.2_changelog

@sunweaver
Copy link
Contributor

@mariogrip @peat-psuwit ^^^

@dobey
Copy link
Contributor Author

dobey commented Jul 26, 2022

As we want to replace lomiri-keyboard by maliit-keyboard in Ubuntu Touch 22.04, this feature will probably dearly be missed. For Debian, it is not so relevant, because people will never have used the presage feature in a Debian stable release.

It worked quite poorly and it is something people commonly turn off as soon as they start using Ubuntu Touch. However, word completion with spell checking via hunspell is still working fine, if the dictionary for the language is installed.

@evelikov
Copy link

Out of curiosity: are there any blockers for removing the presage/prediction databases?

Glancing at my native tongue - Bulgarian - the prediction is very poor or useless even ☹️ Skimming around - Bulgarian and a few other languages have the database_$(lang).db in-tree...

Which as a whole make me wonder if it's actually being maintained?

Thanks o/

@evelikov
Copy link

Even though we ship the SteamDeck with maliit-keyboard, I had to remove the presage integration due to the size and poor UX.

@dobey what's your take on this? Are you split on the topic or waiting for feedback from Debian/Ubuntu folks?

@mikhas
Copy link
Member

mikhas commented Jan 17, 2024

When we first implemented word candidates it was always assumed that better (free!) prediction engines would materialize eventually. I guess that didn't happen so then the whole feature should be dropped...

@evelikov
Copy link

@dobey any chance we can have this merged and a few release please? The latest release was out 1.5 years ago and you've merged a bunch of cool fixes since then.

Thanks in advance, also hope you're OK 🤞

KAMiKAZOW
KAMiKAZOW previously approved these changes Feb 29, 2024
@KAMiKAZOW
Copy link
Member

I do have a merge button now but I these changes are above my skill level to actually judge. I would need confirmation of eg. @evelikov that this still builds and breaks nothing else. I'm also not an owner of this organization, so I cannot elevate eg. someone from the SteamOS developers to (hopefully) maintain Maliit.

@evelikov
Copy link

evelikov commented Mar 2, 2024

Hey @KAMiKAZOW thanks for chipping in.

At a glance, the PR needs a small update to drop some now unused files - I can do that alongside some testing.

That said, I'm not sure what you mean with "to elevate"? If the project is no longer maintained, the README should really be updated to reflect that.

I cannot comment on things from SteamDeck POV, but as an Arch user this PR would be appreciated.

@sunweaver
Copy link
Contributor

@dobey please let us know if you are still upstream for maliit-keyboard + framework. Personally, I have been in touch with Rodney on Maliit issues in Debian + Ubuntu. So, he is around and also using Maliit.

@KAMiKAZOW
Copy link
Member

That said, I'm not sure what you mean with "to elevate"? If the project is no longer maintained, the README should really be updated to reflect that.

To clarify: I have commit access because I write reasonable release notes when someone of the actual developers prepares a release. I'm not and never been in regular contact with any of them.

@dobey
Copy link
Contributor Author

dobey commented Mar 5, 2024

At a glance, the PR needs a small update to drop some now unused files - I can do that alongside some testing.

What do you mean by this @evelikov ? I'm pretty sure I removed all the related files for this here.

@evelikov
Copy link

evelikov commented Mar 6, 2024

At a glance, the PR needs a small update to drop some now unused files - I can do that alongside some testing.

What do you mean by this @evelikov ? I'm pretty sure I removed all the related files for this here.

My initial feeling was that spellpredictworker.cpp and spellpredictworker.h should go

I was off - so is the current code btw. Currently we disable hunspell/spell checker, when presage is off 😱

I think the following files need minor work:

  • .github/workflows/main.yaml - don't install the dev package
  • src/lib/logic/wordengine.cpp - drop Presage references (in comments)
  • tests/autopilot/lomiri_keyboard/tests/test_keyboard.py - drop presage tests
  • tests/unittests/common/wordengineprobe.cpp - drop Presage references (in comments)
  • tests/unittests/ut_editor/wordengineprobe.cpp - as above
  • tests/unittests/ut_preedit-string/ut_preedit-string.cpp - as above
  • tests/unittests/ut_word-candidates/wordengineprobe.cpp - as above

Will prep an updated PR later today, unless someone beats me to it.

As presage has been unmaintained since 2018, among various other concerns with
the feature and how the ngram databases are created, remove text prediction for
now until a better implementation can be done, after keyboard layouts support
has been refactored to be much simpler.
@dobey
Copy link
Contributor Author

dobey commented Mar 7, 2024

Currently we disable hunspell/spell checker, when presage is off 😱

I don't see anything suggesting so in the code. They are separate settings, and separate properties in language plugins. I've also tested the change fairly heavily with hunspell still working.

Will prep an updated PR later today, unless someone beats me to it.

These references were all ineffectual, but I've removed them in this PR now as well.

@evelikov
Copy link

evelikov commented Mar 8, 2024

I don't see anything suggesting so in the code. They are separate settings, and separate properties in language plugins. I've also tested the change fairly heavily with hunspell still working.

I meant that the existing code (aka without this PR) disables hunspell when built without presage... as we can see from the changes to westernlanguagesplugin.cpp and meson.build in this PR.

Although I could be missing something 🤷

In either way, with this PR merged we don't need to care about any of that. Thanks again 🙇

@dobey dobey requested a review from KAMiKAZOW March 11, 2024 19:36
@dobey dobey merged commit 9b64784 into maliit:master Mar 13, 2024
1 check passed
@dobey dobey deleted the drop-presage branch March 13, 2024 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants