Skip to content

Tablet keyboard#328

Merged
andrewtavis merged 17 commits intoscribe-org:mainfrom
bhanu-dev82:tablet_keyboard
Mar 4, 2025
Merged

Tablet keyboard#328
andrewtavis merged 17 commits intoscribe-org:mainfrom
bhanu-dev82:tablet_keyboard

Conversation

@bhanu-dev82
Copy link
Collaborator

@bhanu-dev82 bhanu-dev82 commented Feb 27, 2025

Contributor checklist


Description

This pull request addresses issue #280, implementing layout improvements for different screen sizes and adding enhanced keyboard functionality.

Key Changes:

  • Resource Updates for Different Screen Sizes:

    • app/src/main/res/values-land/dimens.xml: Added values for landscape mode.
    • app/src/main/res/values-w600dp/dimens.xml: Added values for medium tablets.
    • app/src/main/res/values-w1240dp/dimens.xml: Added values for larger tablets.
  • Enhanced Keyboard Functionality:

    • app/src/main/java/be/scri/services/EnglishKeyboardIME.kt: Added custom functions for Caps Lock, Shift, Tab, and Left/Right key handling.
    • app/src/main/java/be/scri/views/KeyboardView.kt: Updated label handling for Caps Lock, Shift, and Tab keys.
    • app/src/main/java/be/scri/helpers/KeyboardBase.kt: Defined custom keys (Caps Lock, Tab, Left/Right arrows).

Testing:

The changes have been tested using the following command:

./gradlew lintKotlin detekt test

Screenshot:

Tablet landscape mode
image

Tablet Portrait mode
image

@github-actions
Copy link

Thank you for the pull request! ❤️

The Scribe-Android team will do our best to address your contribution as soon as we can. If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Android rooms once you're in. Also consider attending our bi-weekly Saturday dev syncs. It'd be great to meet you 😊

@github-actions
Copy link

github-actions bot commented Feb 27, 2025

Maintainer Checklist

The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

  • The linting and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

First PR Commit Check

  • The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution
    - The contributor's name and icon in remote commits should be the same as what appears in the PR
    - If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local Scribe-Android repo (can be set with git config --global user.email "GITHUB_EMAIL")

@angrezichatterbox
Copy link
Member

Thanks for the PR @bhanu-dev82.
Could I get to know which device you are testing this on. Currently on the Pixel Tablet API 35 it doesn't show up the extra keys. Could I get to know from which device onwards it would be using the keyboard you have depicted in the PR description.

@bhanu-dev82
Copy link
Collaborator Author

bhanu-dev82 commented Feb 28, 2025

Thanks for the PR @bhanu-dev82. Could I get to know which device you are testing this on. Currently on the Pixel Tablet API 35 it doesn't show up the extra keys. Could I get to know from which device onwards it would be using the keyboard you have depicted in the PR description.

Hello @angrezichatterbox i have also used pixel tablet api 35, the changes are only for scribe english. i will soon aad the video instructions also. Thankyou

@angrezichatterbox
Copy link
Member

Thanks for the PR @bhanu-dev82. Could I get to know which device you are testing this on. Currently on the Pixel Tablet API 35 it doesn't show up the extra keys. Could I get to know from which device onwards it would be using the keyboard you have depicted in the PR description.

Hello @angrezichatterbox i have also used pixel tablet api 35, the changes are only for scribe english. i will soon aad the video instructions also. Thankyou

Video instruction aren't required. Thanks. We will get to review it soon :)

@bhanu-dev82
Copy link
Collaborator Author

Thanks for the PR @bhanu-dev82. Could I get to know which device you are testing this on. Currently on the Pixel Tablet API 35 it doesn't show up the extra keys. Could I get to know from which device onwards it would be using the keyboard you have depicted in the PR description.

Hello @angrezichatterbox i have also used pixel tablet api 35, the changes are only for scribe english. i will soon aad the video instructions also. Thankyou

Video instruction aren't required. Thanks. We will get to review it soon :)

is the new layout visible i have tested it again by uninstalling and installing it. and it is showing up properly,
image

@angrezichatterbox
Copy link
Member

Can the key sizes be increased a bit for the landscape keyboard The height of the keys to be specific.

Can the key handler be moved to the helper directory and used within the EnglishKeyboardIME as well. Thanks for making the key handlers to a separate class it is really helpful moving forward.

@bhanu-dev82
Copy link
Collaborator Author

Can the key sizes be increased a bit for the landscape keyboard The height of the keys to be specific.

Can the key handler be moved to the helper directory and used within the EnglishKeyboardIME as well. Thanks for making the key handlers to a separate class it is really helpful moving forward.

i will implement both of this today only, how should the new pull request be made? should i delete this one and create a new pull request?

@angrezichatterbox
Copy link
Member

You can push your changes to the same branch. You need not close this PR.

@bhanu-dev82
Copy link
Collaborator Author

bhanu-dev82 commented Feb 28, 2025

You can push your changes to the same branch. You need not close this PR.

To make the inner class a separate file i need to update GeneralKeyboardIME.kt to have some variables and function to use internal instead of protected, also i need to have this file inside the package : be.scri.services as KeyHandler.kt. is it fine if i proceed with this changes ? this approach will keep the encapsulation intact.

@angrezichatterbox
Copy link
Member

You can push your changes to the same branch. You need not close this PR.

To make the inner class a separate file i need to update GeneralKeyboardIME.kt to have some variables and function to use internal instead of protected, also i need to have this file inside the package : be.scri.services as KeyHandler.kt. is it fine if i proceed with this changes ? this approach will keep the encapsulation intact.

Yes it is fine. I would look into the changes once you have made it. The KeyboardIME were getting larger and it would be better if we have it made smaller.

@bhanu-dev82
Copy link
Collaborator Author

You can push your changes to the same branch. You need not close this PR.

To make the inner class a separate file i need to update GeneralKeyboardIME.kt to have some variables and function to use internal instead of protected, also i need to have this file inside the package : be.scri.services as KeyHandler.kt. is it fine if i proceed with this changes ? this approach will keep the encapsulation intact.

Yes it is fine. I would look into the changes once you have made it. The KeyboardIME were getting larger and it would be better if we have it made smaller.

The file KeyHandler has been successfully made and also tested the changes to ensure everything works, for the key height what i have done is calculated the ration in the current implementation and then implemented the same in landscape mode there was a 20 percent increase in key height as seen below in the attached image :
image

@angrezichatterbox
Copy link
Member

It would be better to move the handler to helpers folder right. This would help keep the structure and contents of folders intact. Other than that the changes look good to me.

@andrewtavis
Copy link
Member

Can we make the Scribe key a bit wider in this layout? Ideally the width of the tab key?

@bhanu-dev82
Copy link
Collaborator Author

It would be better to move the handler to helpers folder right. This would help keep the structure and contents of folders intact. Other than that the changes look good to me.

Relocating the handler would require making protected variables public, which would break encapsulation. For this reason, I have kept it within the same package, i.e., services. If modifying the variable's access level is not a concern, I can proceed with moving the file to helpers.

I would appreciate some guidance on the best approach in this scenario. Let me know your thoughts!

@bhanu-dev82
Copy link
Collaborator Author

Can we make the Scribe key a bit wider in this layout? Ideally the width of the tab key?

i am finding solution for it. we need to have changes for tablet / landscape mode only right?

@andrewtavis
Copy link
Member

Yes I'd say for now table / landscape is the important one to change, and we can get to the others later :)

Thanks for looking into this, @bhanu-dev82! 😊

@angrezichatterbox
Copy link
Member

It would be better to move the handler to helpers folder right. This would help keep the structure and contents of folders intact. Other than that the changes look good to me.

Relocating the handler would require making protected variables public, which would break encapsulation. For this reason, I have kept it within the same package, i.e., services. If modifying the variable's access level is not a concern, I can proceed with moving the file to helpers.

I would appreciate some guidance on the best approach in this scenario. Let me know your thoughts!

I would expect the Key handler class to be taking in the private IME of type GeneralKeyboardIME abstract class. Could you tell which variables would be the ones affecting the encapsulation in that case.

@bhanu-dev82
Copy link
Collaborator Author

It would be better to move the handler to helpers folder right. This would help keep the structure and contents of folders intact. Other than that the changes look good to me.

Relocating the handler would require making protected variables public, which would break encapsulation. For this reason, I have kept it within the same package, i.e., services. If modifying the variable's access level is not a concern, I can proceed with moving the file to helpers.
I would appreciate some guidance on the best approach in this scenario. Let me know your thoughts!

I would expect the Key handler class to be taking in the private IME of type GeneralKeyboardIME abstract class. Could you tell which variables would be the ones affecting the encapsulation in that case.

Pardon me i didn't get what you are saying, can you guide me on what should be done to handle this keyhandler.kt?

@angrezichatterbox
Copy link
Member

Sorry if I wasn't clear enough. So currently the Keyhandler.kt class present in it takes only ime of type EnglishKeyboardIME. This could be replaced with the abstract class it is based on that is GeneralKeyboardIME right so that it could accept all the different IME.
Can I know how would encapsulation be broken if the class is moved to another package. The functions would remain private right.

@bhanu-dev82
Copy link
Collaborator Author

Yes I'd say for now table / landscape is the important one to change, and we can get to the others later :)

Thanks for looking into this, @bhanu-dev82! 😊

I have made the scribe button modular with the help of dimens.xml so now it can be increased or decreased in size as we require. i have tried different values till it is of similar width to that of tab key, here is an attached image to show the new UI.
Screenshot_20250228_185834

@andrewtavis
Copy link
Member

Looking wonderful, @bhanu-dev82! Thanks for the quick corrections :)

@bhanu-dev82
Copy link
Collaborator Author

Sorry if I wasn't clear enough. So currently the Keyhandler.kt class present in it takes only ime of type EnglishKeyboardIME. This could be replaced with the abstract class it is based on that is GeneralKeyboardIME right so that it could accept all the different IME. Can I know how would encapsulation be broken if the class is moved to another package. The functions would remain private right.

pardon me again there was some misinterpretation from my side, i have moved the KeyHandler.kt to helpers. and about generalization of the file, i will soon find a solution for it, i am thinking of a modular approach to improve the Code Reusability

@bhanu-dev82
Copy link
Collaborator Author

Sorry if I wasn't clear enough. So currently the Keyhandler.kt class present in it takes only ime of type EnglishKeyboardIME. This could be replaced with the abstract class it is based on that is GeneralKeyboardIME right so that it could accept all the different IME. Can I know how would encapsulation be broken if the class is moved to another package. The functions would remain private right.

I have moved the Keyhandler.kt file to the helpers package and generalized it to support all IMEs. Additionally, I have added icons for all the new keys. I believe this covers everything for this pull request. Please let me know if anything is missing. For each IME, we can create separate issues to discuss and implement necessary changes, as the overall structure is now in place—we would just need to define the key mappings.
image
With caps lock on
image

@angrezichatterbox
Copy link
Member

This could be merged in right. For the other languages we could have a checklist in the issue for the language completed and to be worked on right.

We could also have an issue to improve the styling of command bar for tablets once this is merged in.

@andrewtavis
Copy link
Member

I agree that we can have a checklist in the issue description of #280 rather than making new issues for each keyboard. Do you want to edit the description and add that in, @angrezichatterbox?

Aside from that, happy to do a review myself sometime today 😊

@angrezichatterbox
Copy link
Member

I agree that we can have a checklist in the issue description of #280 rather than making new issues for each keyboard. Do you want to edit the description and add that in, @angrezichatterbox?

I will add that in the issue description

@bhanu-dev82
Copy link
Collaborator Author

This could be merged in right. For the other languages we could have a checklist in the issue for the language completed and to be worked on right.

We could also have an issue to improve the styling of command bar for tablets once this is merged in.

I have reviewed each language layout and determined that updating the keyboard with a tablet-specific layout for each language requires only one XML file per language. since most layouts share common elements, I can complete all eight XML files today.
Additionally, I will include eight screenshots of the updated layouts to ensure this issue is fully addressed.

@andrewtavis
Copy link
Member

Great working with you on this, @bhanu-dev82! 😊

@bhanu-dev82
Copy link
Collaborator Author

bhanu-dev82 commented Mar 2, 2025

Great working with you on this, @bhanu-dev82! 😊

Thank you same here.German keyboard is what is left i am testing it currently will push in a minute

@bhanu-dev82
Copy link
Collaborator Author

I have updated all the languages for tablet layout. Kindly provide feedback. The attached Images are the updated layouts.

English :
image

French :
image

German :
image

Italian :
image

Portuguese :
image

Russian :
image

Spanish :
image

Swedish :
image

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

Commits on my end were to switch the background of the emoji suggestions to transparent :) Thanks for the amazing work here, @bhanu-dev82, and also for the detailed review, @angrezichatterbox! Appreciate the collaboration here from both of you 😊 We can close this and any further changes can be in individual mini issues closer to release 🚀

@andrewtavis andrewtavis merged commit a03dc6c into scribe-org:main Mar 4, 2025
5 checks passed
@bhanu-dev82 bhanu-dev82 deleted the tablet_keyboard branch March 5, 2025 07:45
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.

3 participants