Add option to automatically scale keyboard to screen width #1424
Conversation
6d1b598 to
c4efdd5
Compare
|
This is now working (at least for me) and ready for review. It a little bit of a departure from the previous default sizing experience but I think it makes a lot more sense now. Given that the potentially auto-calculated size is the width not the height of the keyboard, the height is now the value adjusted when non-square mode is activated instead of the width. Also values the user input are now saved as values in the settings database even if the setting is not currently used enabling the use of their previous value from the database when re-enabling a setting. I've tried to migrate existing users to a functional equivalent of their current settings by forcing the new feature off for anybody migrating from an old database, but the actual new feature of sizing based on screen size vs. columns is on by default for new users. I have not tested the behavior of that aspect of the migration yet. How do you handle localization updates? This PR introduces a new string. I've provided the default and TR translation. Do I need to flesh out the other localization files with empty keys or with copies of the unlocalized English or are you using something that will handle that? |
|
Ya the migrations aren't right. Stand by. |
|
I'm not sure what is wrong with the migrations but a basic upgrade to a device previously running db17 causes crashes. This PR does work if you clear data first if anybody wants to test the new features. |
|
I'm trying to sort out what to do with columns and migrations. He're a question: Does it really matter that we even "fix" the columns in the database schema so they have the right defaults? Even if we stop allowing NULL, won't it work out if we just relpace any existing NULLs and then change the usage so we never set another NULL? As in the slider values should always have a value now even if they are not being used at that moment due to some other sizing being in effect. It rubs me the wrong way to have the schema not reflect actual usage, but so does having defunct columns we can't drop or use and new tables with names that don't match their data mapping. Speaking of dropping, that appears to be possible in SQLite, is it Room that won't let us do that? |
Correct, but not just room, android in general. You can read some of the comments here, but SQLITE only added drop column in 2021, and not all android versions support the newest sqlite. I know its annoying, but bc of this I've found its safer and harmless to just have defunct columns around, and only add new column at the end. Changing data types also tends to throw errors. And only if absolutely necessary, do a whole new create table migration with corrected columns and copy the old data over. The steps forward are:
|
|
Thanks for the feedback, I'm working towards that now. The currently pushed commit (13cf77a) does NOT work, it crashes on opening a v17 database, presumably something to do with migrations. It works fine on its own with data cleared. I've also tried it without the four Unfortunately the debug log I'm seeing with On a quick gander now do you see any obvious bloopers? |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Hrm... none of those errors are DB-related, although they could be downstream. |
Yes I didn't recognize them as being remotely related to what I was working on. The crash definitely happens (and is reproducible for me on two wildly different devices) any time I open a settings db from the last released version in this branch, while it seems to run fine with the settings cleared. |
app/src/main/java/com/dessalines/thumbkey/ui/components/keyboard/KeyboardScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/dessalines/thumbkey/ui/components/keyboard/KeyboardScreen.kt
Show resolved
Hide resolved
4db75c7 to
8cd3fd5
Compare
d501d29 to
e25bb6c
Compare
|
Run LMK when you're ready for me to pull this down and test, or if you can't get past a bug. |
Thanks for the patience on this one. I think I've finally squashed the migration bugs and I can reliably set settings in a build from main, then open a build of this PR and have the relevant settings come in without crashes. I think this is ready for actual review with an eye to merger. |
|
I'm marking this as draft again, hopefully briefly, because I see you merged #1443 already and the column calculation needs to be adapted here to do something sensible in the event of double-the-trouble. |
Pull request was converted to draft
|
And we're back in business. When the dual position is activated we double the expected column count and adjust the width so that both layouts fit. |
There was a problem hiding this comment.
Tested and everything works great, including migrations, and the settings. thx a ton for this.
I'm a little bit iffy on having the auto-width being default (for example I tested this on my tablet in landscape mode and it took up the entire screen and was unusable. Any device in landscape mode, or where the width >= height will have that problem). But I think having auto-width default is still probably the best option for 99% of users, and the others can use defined sizes.
I'll make a release for this shortly.
|
Yes, the default for landscape mode anything is a bit aggressive, especially with the typical 4x4 square layout. It's marginally better for some of the new wider layouts or the dual mode, which is probably what you want for a tablet in landscape mode anyway. That or setup a corner layout with no background. But I agree defaulting to something nice for phones and typical usage made sense to me, and whatever tweaks need to be made for other situations seemed like an okay cost. |
Closes #1423
At the moment this is just a work in progress that takes over the size setting and does the math as a POC. No matter what size you set the slider it will just recalculate it. UI changes yet to come.
Contributions welcome if anybody wants to fiddle with this against my fork & branch.