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

Fix toolbar keys order in RTL layout #574

Merged
merged 10 commits into from
Mar 27, 2024
Merged

Conversation

codokie
Copy link
Contributor

@codokie codokie commented Mar 18, 2024

Fix the toolbar keys direction so that it does not change when switching between an RTL/LTR layouts.
The issue was described here: #557
Upon testing the app in Android Studio, the order seems to be fixed with no adverse effect on the direction of the text in the auto-complete (RTL layout) suggestions.

@codokie codokie closed this Mar 19, 2024
@codokie codokie deleted the rtl-toolbar-fix branch March 19, 2024 10:48
@codokie codokie restored the rtl-toolbar-fix branch March 19, 2024 10:54
@codokie
Copy link
Contributor Author

codokie commented Mar 19, 2024

Commit 1eac01e has a reference to PR number 2, but it is unrelated to it.
Please excuse me as I am new to GitHub, apparently commit messages may not be edited.
I am also sorry for the relatively high number of commits. if it is preferred, I can close this PR and reopen a new one with a single commit.
in case quirks that exist in GBoard are to be kept in HeliBoar, I would be grateful if someone could implement a UI toggle in settings to make the toolbar direction fixed for RTL languages.

@codokie codokie reopened this Mar 19, 2024
@Helium314 Helium314 linked an issue Mar 23, 2024 that may be closed by this pull request
@Helium314
Copy link
Owner

Please excuse me as I am new to GitHub, apparently commit messages may not be edited.

That's normal in git, and usually they should not be edited.
Having additional "useless" commits is not an issue, when merging I squash everything into a single commit anyway.

I see you didn't remove setRtl, it still adjusts the layout direction of mStripVisibilityGroup. Is this useful / needed, or can it be removed too?

Did you test the arrow keys in the toolbar, and when pinned? Just to be sure they point in the correct direction.

in case quirks that exist in GBoard are to be kept in HeliBoar, I would be grateful if someone could implement a UI toggle in settings to make the toolbar direction fixed for RTL languages.

I'm ok with merging this without toggle, but would you be willing to try adding one later, in case someone complains about this change?

@codokie
Copy link
Contributor Author

codokie commented Mar 23, 2024

Having additional "useless" commits is not an issue, when merging I squash everything into a single commit anyway.

That's neat, I did not know you could combine those together. Thanks for the reassurance :)

would you be willing to try adding one later, in case someone complains about this change?

So I actually learned how to add a toggle myself, it was not really complicated. I added the toggle to the Preferences screen.
(I did some testing and so far it seems to work fine..)

Did you test the arrow keys in the toolbar, and when pinned?

Arrow keys/pinned toolbar buttons point in the same direction as before if the toggle is off.
If it is turned on, then the direction of all toolbar items is determined by the system locale.

I see you didn't remove setRtl, it still adjusts the layout direction of mStripVisibilityGroup. Is this useful / needed, or can it be removed too?

In the 845e6f9 commit I've moved some of the code of the setLayoutDirection method to setRtl, because the former belongs to a static class and the mRtl variable (which has been restored) could not be referenced from it. So setRtl is still needed I think.

@Helium314
Copy link
Owner

Thanks, this is fine now.

My only issue is with the name and description of the setting. The name is ambiguous, and it's completely unclear that the setting is about ltr / rtl languages.

@Helium314
Copy link
Owner

Maybe something like "toolbar direction language direction" (though I'm not really happy with that one as well).

@jermanuts
Copy link

jermanuts commented Mar 25, 2024

what about "toolbar direction"?

description: Enable it for RTL direction layout, If disabled, the direction of the active layout will be used (LTR).

@Helium314
Copy link
Owner

I would like to avoid RTL / LTR shortcuts as it would be confusing to many people.

How about reversing the setting and use Variable toolbar direction, with some description like Reverse toolbar (or toolbar direction?) for right-to-left scripts (languages?)

@codokie
Copy link
Contributor Author

codokie commented Mar 26, 2024

Thank you both for your input.
I changed it to: "Variable toolbar direction"
with description: "Reverse direction when a right-to-left keyboard subtype is selected"
Hopefully that makes sense. If not, please feel free to change it.

@Helium314 Helium314 merged commit fb9008e into Helium314:main Mar 27, 2024
1 check passed
@codokie codokie deleted the rtl-toolbar-fix branch April 14, 2024 11:32
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.

Toolbar keys order is reversed
3 participants