Code improvements to make Scribe ready for future features

[linreal]

Terms

• [x] I have searched open and closed feature requests
• [x] I agree to follow Scribe-Android's Code of Conduct

Description

After working on Add invalid state to keyboard commands and show localized Not in Wikidata and app crashing at startup on api 30 I've noticed some issues, which could lead to maintenance problems in future, and also push away potential maintainers.

Specifically, I see problems with GeneralKeyboardIME and and classes inheriting it.

Main things that, in my opinion, should be addressed:

• [ ] GeneralKeyboardIME mixes view management, database access, state handling, and business logic in ~2,100 lines. The suppression of TooManyFunctions and LargeClass hints at maintainability issues. This makes it difficult for potential contributors to understand or extend the code.
• [ ] isTablet duplicates in every subclass
• [ ] heavy use of non-null assertions (!!) in the subclasses when setting up the keyboard view
• [ ] fuzzy state management without stateholder
• [ ] base class knows about classes inheriting it (i.e baseKeyboardOfAnyLanguage function and many others)
• [ ] languages logic is based on strings comparison instead enums or sealed classes (i.e getLanguageAlias function)
• [ ] various code style issues (like using "when" in setupCommandBarTheme function), invalid log tags (like "my-tag") and so on

Contribution

I would like to discuss if necessary and fix some of listed problems in one (or multiple) separate PR's Let me know, if you ok with it

[andrewtavis]

Amazing to have this issue, @linreal! Thank you so much for the thorough review of the application 🙏 Your points definitely make sense to me 😊

CC @angrezichatterbox and @bhanu-dev82 for the conversation here. Let's wait until #411 is in before we work on any of the above, but we could convert the above list into a checklist and then work through it in multiple PRs :)

[angrezichatterbox]

Happy to have a discussion on this. The GeneralKeyboardIME should be refactored at this point. I thought of ending the Conjugate feature and then move to refactoring. But happy to discuss on it. We should also look into standardizing the labels mode switch while this is being done.

[angrezichatterbox]

@linreal Happy to have a discussion on this on a call on elements or however you would like. @andrewtavis I think that would be better right ?

[andrewtavis]

Happy to have a call on this!

[andrewtavis]

Hey @linreal 👋 Would you have interest in joining the Scribe developer sync this upcoming Saturday at 15:00 UTC? Announcements for that are in the General channel on Matrix, with the reminder going out tomorrow that will share the meeting link :) We could spend some time there discussing this issue. Also happy to do a call another time, or we can also make sub issues async!

[linreal]

Hey @linreal 👋 Would you have interest in joining the Scribe developer sync this upcoming Saturday at 15:00 UTC? Announcements for that are in the General channel on Matrix, with the reminder going out tomorrow that will share the meeting link :) We could spend some time there discussing this issue. Also happy to do a call another time, or we can also make sub issues async!

@andrewtavis @angrezichatterbox gentlemen, I'm very sorry for delayed replies - got buried with work tasks.

In my opinion, the main (and also, the most complicated) problem of the above list is the management of the keyboard state, which should be carried out in StateHolder. All others could be relatively easy fixed in separate issues (correct me if you not agree)

I'll try to connect to call on Saturday, but if not - then I will start working more thoughtfully on this from next week

[linreal]

Wow, wow, missclick! @andrewtavis can you reopen this, please?

[andrewtavis]

Sounds good on my end, @linreal! Hope to chat with you about this Saturday, but let's keep in touch async if not. I converted the points in your issue text to checkboxes, btw, just so we're able to track them being finished 😊

[Nishthajain7]

Can I take this issue up?

[Nishthajain7]

Thank you !

[andrewtavis]

You'd be welcome to, @Nishthajain7! Let us know which one of the options in the issue description you'd like to work on :)

[Nishthajain7]

I'd like to work on the GeneralKeyboardIME since I've worked with it already for the auto suggestion issue and I am familiar with the code there

[andrewtavis]

Sounds perfect to me, @Nishthajain7! Feel free to open up a PR, and please let us know if you need some support! 😊

[Nishthajain7]

So when I see a lot of code related to the same functionality, I am moving them to a separate file. I moved all the Conjugate View related code to a conjugateHandler in helpers.

I was wondering if I should create a new file to handle backspace key press (like spaceKeyProcessor) or handle it in keyHandler itself

[andrewtavis]

Let's maybe wait on switching up the backspace right now, but do you want to make an issue for that specifically and explain the benefits of it? You'd be welcome to work on it, but just to make sure that we don't expand this issue out and the first PR too much 😊

[Nishthajain7]

You mean I make a seperate Issue on refactoring of GeneralKeyboardIME or the backspace handlers?

[andrewtavis]

For the backspace handler :)