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 space (height) between keys #8

Merged
merged 2 commits into from
Jul 23, 2023
Merged

Conversation

gigisforza70
Copy link

fix space between keys, make them like gboard,for better user experience.

fix space between keys, make them like gboard,for better user experience.
@RHJihan
Copy link
Contributor

RHJihan commented Jul 8, 2023

@Helium314, it's a good one. Consider merging this PR.

@Helium314
Copy link
Owner

So what does it actually do? So far I didn't consider the height between keys to be bad user experience or broken / in need of fixing.

@Helium314
Copy link
Owner

Is there any issue in OpenBoard about this?

@RHJihan
Copy link
Contributor

RHJihan commented Jul 10, 2023

It reduces the key travel distance. I used both versions of the keyboard. I found that it's more comfortable and became sleek looking with less key distance. It does not confirm an obvious good user experience, but a proof of concept of it.

23b19ab on OpenBoard contains these changes, which was never merged.

@odmfl
Copy link

odmfl commented Jul 10, 2023

Yes, I had also made these changes in the emoji commit.
However, in my opinion, the user experience improves a lot, typing is much more comfortable. Please merge this commit

@Helium314
Copy link
Owner

So I tested this, and my question is what's the difference between this and setting the keyboard height to 90% in appearance settings?

@gigisforza70
Copy link
Author

beyond the keyboard height, which can be changed in the settings. This commit reduces the space between keyboard rows, making it more comfortable. I leave you 2 screens
43AE64C6-5D55-43BA-9EC5-05F1692EB3A4
FBB84D9B-A0C0-44B7-BD69-9F93D22695B1

@Helium314
Copy link
Owner

I still don't get it. One of the screenshots shows effect of the PR, the other one shows setting keyboard height to 90%.

90
pr

@gigisforza70
Copy link
Author

Very strange, your screens look the same; while mine with 90% height and with the PR are different. Perhaps you are using a more up-to-date version of the app, you could publicly release the version you are using also in the debug version, so as to try it. Maybe it was fixed and we didn't even notice it.

@gigisforza70
Copy link
Author

even if adding the row of numbers looks the same?

@RHJihan
Copy link
Contributor

RHJihan commented Jul 13, 2023

Basically, keyboard resizing changes the key size and; ultimately changes the whole keyboard height. This PR is to change the space between rows.
‍ ‍

Built with this PR this fork this fork (90% keyboard size)

@gigisforza70
Copy link
Author

Basically, keyboard resizing changes the key size and; ultimately changes the whole keyboard height. This PR is to change the space between rows. ‍ ‍


Built with this PR this fork this fork (90% keyboard size)

good, now we have an overview.

@Helium314
Copy link
Owner

Basically, keyboard resizing changes the key size and; ultimately changes the whole keyboard height. This PR is to change the space between rows. ‍ ‍

Thanks for doing something that should have been in the PR since the start. (I'm honestly considering just closing such badly described PRs in the future)

Anyway, on my phone this PR changes the keyboard height. It could be that it depends on Android version or device, but this should definitely not happen for anyone.

I'm ok with the key height change though.

@RHJihan
Copy link
Contributor

RHJihan commented Jul 14, 2023

Will this PR be getting merged?

@Helium314
Copy link
Owner

If I get a version that changes key spacing without affecting the height on any device, sure.

Btw if this is only about key spacing, what is the change to config_emoji_keyboard_row_height doing? It doesn't seem related.

@RHJihan
Copy link
Contributor

RHJihan commented Jul 15, 2023

I tested this PR on different emulators on Android Studio. All reflect this change perfectly.

config_emoji_keyboard_row_height enlarges the emoji size. It's better for typing.

Built with this PR this fork

@Helium314
Copy link
Owner

This is what it looks like for me: (PR on the left)
s
Just from this I can't tell whether the differences come from config_emoji_keyboard_row_height or from the general keyboard height change.

So may request to provide a version of this PR that does not affect keyboard height remains.

@RHJihan
Copy link
Contributor

RHJihan commented Jul 17, 2023

From my build of this PR, config_emoji_keyboard_row_height does not affect the overall keyboard height.

config_emoji_keyboard_row_height = 50 config_emoji_keyboard_row_height = 30

@odmfl
Copy link

odmfl commented Jul 17, 2023

@Helium314 what android device do you have? Everything seems to work differently on your device, maybe it's the radio aspect, or maybe the android version, but it seems like your device works differently than others. We need to investigate if maybe others also have the same problems

@Helium314
Copy link
Owner

I use a Samsung S4 Mini

@odmfl
Copy link

odmfl commented Jul 17, 2023

I use a Samsung S4 Mini

ok, so in my opinion it affects both the android version and also the small screen. I assure you that the changes in this commit and as demonstrated by the screenshots also improve the daily use experience of the keyboard. Apparently the old phones do not undergo any changes

@BabyOilJohnson
Copy link

BabyOilJohnson commented Jul 22, 2023

Will this PR gets merged?

@Helium314
Copy link
Owner

see #8 (comment)

@gigisforza70
Copy link
Author

see #8 (comment)

I restored the old height, actually how it was changed it was like 90%. Now it should be fine.

Looking at the issue #30 the solution is always found in the same file, under the entry "config_emoji_keyboard_key_width" if this value goes to 16.666%p, the "emoji keyboard" height becomes equal to that of the keyboard without the number row. The problem is that when you activate the number row, the height of the "emoji keyboard" does not change

@Helium314 Helium314 merged commit 0834f40 into Helium314:new Jul 23, 2023
@Helium314
Copy link
Owner

Thanks, this works nicely now!

@ChuckBartowski91
Copy link

ChuckBartowski91 commented Jul 30, 2023

Basically, keyboard resizing changes the key size and; ultimately changes the whole keyboard height. This PR is to change the space between rows. ‍ ‍

Built with this PR this fork this fork (90% keyboard size)

I would like to be able to keep the space between keys because is less prone to errors, al least be able to change the spacing with an option.

@Helium314
Copy link
Owner

So it looks like the reasoning "it's good experience" (and similar) doesn't apply to all users...

@gigisforza70
Copy link
Author

Basically, keyboard resizing changes the key size and; ultimately changes the whole keyboard height. This PR is to change the space between rows. ‍ ‍

Built with this PR this fork this fork (90% keyboard size)

I would like to be able to keep the space between keys because is less prone to errors, al least be able to change the spacing with an option.

Currently the spaces are tighter, but the keys are bigger, you have more space to press the keys, so how do you get typos? With this the typing errors are halved. Then if the errors are given by the height of the keyboard that can be changed.

@ChuckBartowski91
Copy link

Basically, keyboard resizing changes the key size and; ultimately changes the whole keyboard height. This PR is to change the space between rows. ‍ ‍

Built with this PR this fork this fork (90% keyboard size)

I would like to be able to keep the space between keys because is less prone to errors, al least be able to change the spacing with an option.

Currently the spaces are tighter, but the keys are bigger, you have more space to press the keys, so how do you get typos? With this the typing errors are halved. Then if the errors are given by the height of the keyboard that can be changed.

Probably because having less space between keys if I'm not extremely precise can register that I'm pressing a button next to the one I wanted to press, instead with more spacing if I tap on the edge of a key there is less chance that i will insert the wrong character.

@gigisforza70
Copy link
Author

@ChuckBartowski91 Yes, but the keys are larger, so it substantially reduces the chance of randomly pressing neighboring keys. The wide keys as before were only on the Samsung keyboard (unwatchable and almost unusable)

@ChuckBartowski91
Copy link

@ChuckBartowski91 Yes, but the keys are larger, so it substantially reduces the chance of randomly pressing neighboring keys. The wide keys as before were only on the Samsung keyboard (unwatchable and almost unusable)

I understood that, bigger keys means more space to press but means too that the finger can easily choose the next key if you press the edges of the keys.

@BlackyHawky
Copy link
Contributor

Am I the only one who finds emojis really too big since this PR was merged?

I don't understand the link between reducing the distance between the keys (which I think was necessary) and increasing the size of the emojis...

@Helium314
Copy link
Owner

Am I the only one who finds emojis really too big since this PR was merged?

This may have to do with the device? Not sure...

I've been looking at making the changes to key gap optional, since obivously not everyone is happy with them.
Should not be too hard, the only tricky thing is properly dealing with larger screens and landscape mode, where apparently the key gap change is not wanted (at least the files were not touched in this PR).

@Helium314
Copy link
Owner

This is now optional, using config_key_vertical_gap_holo_narrow and config_key_horizontal_gap_holo_narrow.
For landscape and higher smallest width resources, the new values are set to the current non-narrow value. Feel free to adjust values for these variants.

@BlackyHawky there are two independent things here increasing emoji size: increasing config_emoji_keyboard_row_height and decreasing the gaps. You can try undoing the config_emoji_keyboard_row_height change and see how it looks.

@BlackyHawky
Copy link
Contributor

BlackyHawky commented Aug 14, 2023

It's much better with the old "config_emoji_keyboard_row_height" value, but that's just a personal opinion, so don't take my comment on board. I've got used to it a bit.

Two question though:

  • I guess we could have the same option for emojis?
  • Or maybe set its own values in the app? Values between 1 and 10 for example.

In any case, thank you for this update. Many will appreciate it.

@Helium314
Copy link
Owner

I guess we could have the same option for emojis?

Not for this, it is much less noticeable than the key gaps.
But actually I would be ok with going back to the old value, or maybe something like 29% or 28.5%.

Or maybe set its own values in the app? Values between 1 and 10 for example.

I wanted to allow this, but the values are different for many of the config variants, which complicates things a lot. Normally I really dislike adding more xml attributes, but here it seemed like the most feasible choice.

@BlackyHawky
Copy link
Contributor

Not for this, it is much less noticeable than the key gaps. But actually I would be ok with going back to the old value, or maybe something like 29% or 28.5%.

In my opinion it is preferable because from what I have seen and tested:
Reducing the gap between the keys increases the size of the emojis and this PR does both → therefore illogical to modify <fraction name="config_emoji_keyboard_row_height">

Or maybe set its own values in the app? Values between 1 and 10 for example.

I wanted to allow this, but the values are different for many of the config variants, which complicates things a lot. Normally I really dislike adding more xml attributes, but here it seemed like the most feasible choice.

As you wrote in the Readme first priority is to clean these xml files.
You will see later. 😉

@Helium314
Copy link
Owner

As you wrote in the Readme first priority is to clean these xml files.

I noticed there are some duplicates, and some unused values.

Also, I'd like to move the keyboard layouts either completely away from xmls, or at least massively simplify them.
One idea is to have some xml parameters determining the layout, i.e. arrangement of keys, only. They actual keyboard layouts would then only contain the keys (letter + moreKeys), and the spacing and stuff is done in code using values from xmls (depending on screen size, orientation, split setting,...).

If it works out, this would remove the need for separate resources for each keyboard layout for split, landscape, tablet,...

@RHJihan
Copy link
Contributor

RHJihan commented Aug 20, 2023

Am I the only one who finds emojis really too big since this PR was merged?

Yes, the emojis do look big since this PR was merged.
The emojis would look perfect with config_emoji_keyboard_row_height = 30%p, if this commit was merged.

 <fraction name="config_max_keyboard_height">49%p</fraction>
 <fraction name="config_min_keyboard_height">-54.8%p</fraction>

Later this change was skipped in this PR.

Device: Galaxy M32 (Android 13)
Surprisingly I couldn't reproduce it on the emulator. The emoji size stays the same on there.

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.

7 participants