-
Notifications
You must be signed in to change notification settings - Fork 211
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
Implement non-square keys #692
Conversation
I was mostly thinking towards the direction of making it one of the toggles parameters, which maxes out width but still can change the actual key height through the existing key size param. That way backward compat would be preserved too, I think. Good job though, that was quick! |
Since you're already working on this part of the code, can you add a slider for the horizontal position of the keyboard? (instead of the current left/center/right options available) |
this would also technically solve #653 since you could submit multiple layout sizes, although having to hard code the different layout sizes yourself will be annoying. |
@Zwyx unfortunately your build has failed, but I'd really like to try this. could you fix the formatting so that the build succeeds and we can get the artifacts from it? idk how to build this myself. |
8033521
to
cb59974
Compare
It's done @RainOrigami, however I'm actually interested to know where you retrieve the artifacts?! I can't find them in Woodpecker but I'm not familiar with it. |
Hm yes you raise a fair point, I would have expected some to show up when the build task completed. I guess I have to figure out how to build this 😵 edit: oh no this is way harder than I have anticipated, I have to like sign this file too and shit edit2: ok thank you, I am now beyond happy and this makes it the perfect replacement for messagease! @Zwyx to note, there are some errors still that prevent the assembleRelease build from gradle, but not the assembleDebug build. I suppose the debug build doesn't contain translations? Anyways, there are Or I do not know how to properly build a release, anyways once confirmed that this PR doesn't break anything, pleasepleasepleaseplease @dessalines accept this PR <3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a migration.
@@ -61,10 +62,15 @@ const val DEFAULT_KEY_RADIUS = 0 | |||
data class AppSettings( | |||
@PrimaryKey(autoGenerate = true) val id: Int, | |||
@ColumnInfo( | |||
name = "key_size", | |||
defaultValue = DEFAULT_KEY_SIZE.toString(), | |||
name = "key_height", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't change the names of existing columns, or you'll break installs.
You'll need to do a migration to add more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main recommendation for this PR, is that height and weight should by default be tied together. Add a boolean setting (that doesn't actually link to anything in the DB), for something like "Use different height and width", which is shown either if they're different.
And then show both sliders. Otherwise, the first slider should change both the height and width as if they were the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @dessalines, happy to see that you approve the idea :-) The boolean setting is a great idea! I'll work on that.
For the names of the columns, I will keep key_size
and add key_width
.
by splitting `key_size` in `key_height` and `key_width`
5e7ca81
to
108caa7
Compare
108caa7
to
53c01b1
Compare
I have implemented what we discussed:
I have tested that this works for both a fresh install, and an update (the DB migration works). @RainOrigami, you can do |
Nah that'd probably require an IDE and like 60GB worth of SDKs and libraries. The docker approach worked for me, I did end up building the debug configuration instead of release, which signs the apk and lets me install it 👌 Good enough temporarily and seeing how you knocked this feature out of the park, I can probably expect this to be in the next release fingers crossed! Nice work! |
Thanks! Ah yes it does require all that. Could you share more details about how you did it with Docker? |
Resulting APK is in |
Oh excellent! You took it from the Woodpecker file, makes complete sense. Love it. Thanks for that 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well, but see if you can make key_width
a nullable int column, as that makes more sense.
@@ -130,17 +130,19 @@ fun KeyboardScreen( | |||
val backdropColor = MaterialTheme.colorScheme.background | |||
val backdropPadding = 6.dp | |||
val keyPadding = settings?.keyPadding ?: DEFAULT_KEY_PADDING | |||
val legendSize = settings?.keySize ?: DEFAULT_KEY_SIZE | |||
val legendHeight = settings?.keySize ?: DEFAULT_KEY_SIZE | |||
val legendWidth = (if ((settings?.keyWidth ?: 0) > 0) settings?.keyWidth else legendHeight) ?: legendHeight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can tell that 0 is being used as a sub for null here, otherwise this could be as easy as
val legendWidth = settings?.keyWidth ?: legendHeight
object : Migration(13, 14) { | ||
override fun migrate(db: SupportSQLiteDatabase) { | ||
db.execSQL( | ||
"alter table AppSettings add column key_width INTEGER NOT NULL default 0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on some of the logic below, 0 is being used as a sub for null. Really this should be just
alter table AppSettings add column key_width Integer
which should make it of type Int?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes complete sense indeed, I'll do that. Sorry I'm new to Kotlin and my work is based on copy-paste of existing code 😬 Thanks for your patience 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No probs!
Ok @dessalines, it's done 🙂 I had two questions:
|
I haven't noticed the black screen, you'll have to open an issue for how to replicate it. I don't use github sponsors, but have liberapay and patreon. Liberapay is much preferred since they're open source themselves, don't take a cut, and are funded by their own model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well, thx!
Some things for future PRs:
|
Thank you, donation set up on Liberapay 👍 Thanks for making Thumb-Key! I take note of these two things for future PRs. |
No probs, thx for this! |
Implement non-square keys
by splitting
key_size
inkey_height
andkey_width
I think having
key height
andkey width
allows more precision than aratio
setting. It also doesn't require to know the screen width and the number of keys in the keyboard.Notes
Left to be done, if we choose to go ahead with this proposal
key_size
.