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

Add toolbar option to toggle Split Keyboard Setting - Enhancement #1218 #1263

Merged
merged 13 commits into from
Feb 9, 2025

Conversation

PurplePickleMonster
Copy link
Contributor

@PurplePickleMonster PurplePickleMonster commented Jan 4, 2025

Enhancement: #1218

Add toolbar option to toggle Split Keyboard Setting.

Screen_recording_20250103_233551.webm

@PurplePickleMonster PurplePickleMonster changed the title [DRAFT] Add toolbar option to toggle Split Keyboard Setting Add toolbar option to toggle Split Keyboard Setting Jan 4, 2025
@PurplePickleMonster PurplePickleMonster marked this pull request as ready for review January 4, 2025 04:42
@PurplePickleMonster PurplePickleMonster changed the title Add toolbar option to toggle Split Keyboard Setting Add toolbar option to toggle Split Keyboard Setting - Enhancement #1218 Jan 4, 2025
@Helium314
Copy link
Owner

Thanks, this looks good.
I'll have a closer look in the next few days.

Is there a technical reason for split and merge being separate key codes, or could it be changed to a toggle?

@PurplePickleMonster
Copy link
Contributor Author

Thanks, this looks good. I'll have a closer look in the next few days.

Is there a technical reason for split and merge being separate key codes, or could it be changed to a toggle?

Sounds good. Yes I believe this can be a toggle instead. I will look into updating this week.

@Helium314
Copy link
Owner

Yes I believe this can be a toggle instead

On first glance it looked like it should work, so I wasn't sure why you would take the 2 keycode approach.
But now I see I did the same for one-handed mode, which was done for historic reasons... I guess I'll also change it into a single code later.

@PurplePickleMonster
Copy link
Contributor Author

MERGE_LAYOUT Keycode removed and SPLIT_LAYOUT is now a toggle of the SplitKeyboardEnabled Setting

Comment on lines 82 to 83
KeyCode.SWITCH_ONE_HANDED_MODE
KeyCode.SWITCH_ONE_HANDED_MODE,
KeyCode.SPLIT_LAYOUT
Copy link
Owner

Choose a reason for hiding this comment

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

This is not necessary. Currently it does nothing, because DEFAULT is only accessed by index of ID_TO_NAME, and you do not add an entry to ID_TO_NAME.

I recommend to just remove it again and leave KeyboardCodesSet untouched, as it is only used for the "old" label + code style like a|!code/key_action_previous, which only exists for historic reasons (I don't want to break people's layouts by removing it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted change, KeyboardCodesSet is no longer modified

@Helium314
Copy link
Owner

The SPLIT_LAYOUT code needs to be added to checkAndConvertCode in KeyCode.kt, so people can define the split key (code) anywhere they want.
Looking at KeyCode.kt I see why you chose the split / merge code approach. I didn't notice they were both in the FlorisBoard codes. But I think a single code is enough.

@Helium314
Copy link
Owner

Could you also set the activation state of the SPLIT key in ToolbarUtils.createToolbarKey?

@Helium314
Copy link
Owner

I noticed an issue that makes things a little more complicated: the split button does nothing if the screen width is under 600 dp (as per SettingsValues).
This will certainly be reported as a but, and needs to be addressed.

Currently I see 2 ways:

  1. hiding the split button when the width is < 600 dp
    but I fear this will also look like a bug to some users (split button available in toolbar settings, but not on toolbar, and other things with custom split key and device orientation)
  2. drop the 600 dp requirement
    this will break things for people who currently have a normal keyboard in portrait orientation, and a split keyboard in landscape. Only solution I see here is to "split the split" into portrait and landscape, and have the toolbar key toggle split for the current orientation.

Apply suggestions

Co-authored-by: Helium314 <[email protected]>
PurplePickleMonster and others added 2 commits January 7, 2025 17:38
…al settings for split keyboard based upon orientation.
@PurplePickleMonster
Copy link
Contributor Author

@Helium314 That was difficult... but it works... 🥴 Toggle now controls split layout only for current orientation. There are now two settings options, one for each orientation.

@Helium314
Copy link
Owner

Thanks for adding this.

That was difficult...

Sorry, it was not meant to be some sort of time-wasting exercise. I should have mentioned that you can simply ask me if you have some difficulties.

Anyway, could you still adjust the split code a little?
I'd like to handle the display orientation in SettingsValues similar to mOneHandedModeEnabled = Settings.readOneHandedModeEnabled(prefs, mDisplayOrientation == Configuration.ORIENTATION_PORTRAIT), so there is no need to change anything that queries mIsSplitKeyboardEnabled. Well, except for appearance settings of course.

Do you think the settings for mSplitKeyboardSpacerRelativeWidth also need to be split?
I can imagine users might want it to be different, but also I don't use the split keyboard at all. Anyway it's your decision and it can also be added later.

Comment on lines 197 to 200
? Math.min(Math.max((displayWidthDp - 600) / 600f + 0.15f, 0.15f), 0.35f) * prefs.getFloat(Settings.PREF_SPLIT_SPACER_SCALE, DEFAULT_SIZE_SCALE)
mSplitKeyboardPortraitSpacerRelativeWidth = mIsSplitKeyboardPortraitEnabled
? Math.min(Math.max((displayWidthDp) / 0.15f, 0.15f), 0.35f) * prefs.getFloat(Settings.PREF_SPLIT_SPACER_SCALE, DEFAULT_SIZE_SCALE)
Copy link
Owner

Choose a reason for hiding this comment

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

Math.min(Math.max((displayWidthDp) / 0.15f, 0.15f), 0.35f) is always 0.35 if your display is more than 1 dp wide.
I think the old method should still be used, so users don't have to re-adjust the split width setting.

@Helium314 Helium314 merged commit 679754b into Helium314:main Feb 9, 2025
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.

2 participants