Modals, Button Presses, Keyboard update, Progress Bar#114
Conversation
There was a problem hiding this comment.
I'm very much a Kotlin noob so this is probably a dumb question, but why not use a multiline string for this?
There was a problem hiding this comment.
great Q honestly, that's one of the features I always forget about. I used appendLine for the dynamic stuff and then just duped the line a bunch. Will change.
| } | ||
|
|
||
| private val Int.isSystemKeyCode: Boolean | ||
| get() = (this == KeyEvent.KEYCODE_BACK || this == KeyEvent.KEYCODE_HOME) |
There was a problem hiding this comment.
What's the circumstance when the Android BACK button will actually be emitted, given that the LP3 doesn't have a hardware one?
There was a problem hiding this comment.
back handling is mostly for emulator, external keyboard users, or if folks want to ship their tools on non-light devices!
Also, probably flip!!
| displayVoice = result.displayVoice, | ||
| enableKeyAnimation = result.enableKeyAnimation | ||
| enableKeyAnimation = result.enableKeyAnimation, | ||
| swipeEnabled = false |
There was a problem hiding this comment.
This is necessary because of the keyboard version bump? Will this ever be updated to true?
There was a problem hiding this comment.
Great callout - I think I was avoiding adding a new field to the response before necessary but now's probably the perfect time to do it.
| runCatching { ctx.unbindService(this) } | ||
| .onFailure { Log.w(TAG, "Failed to unbind service", it) } | ||
| bound = false | ||
| bind(ctx, pkg) |
There was a problem hiding this comment.
I'm unfamiliar with this lifecycle, could this error loop?
There was a problem hiding this comment.
to my knowledge, no - bind calls bindService which just returns false if it fails.
| } | ||
|
|
||
| fun onDeviceKeyEventRequest(request: LightServiceMethod.DeviceKeyEvent.Request): AudioModal? { | ||
| if (request.action != ACTION_DOWN) return null |
There was a problem hiding this comment.
This is again probably due to my lack of context, but would this handle the case where the user is just holding the button down to move the volume up/down repeatedly, ie key repeat?
There was a problem hiding this comment.
yep, ACTION_DOWN is sent repeatedly with a repeatCount field.
| } | ||
|
|
||
| // Modeled after LightOS volume rocker modal | ||
| // In ideal world, this will be used directly in LightOS |
There was a problem hiding this comment.
Meaning this would be factored out into a tiny standalone package that LightOS can import directly?
There was a problem hiding this comment.
yep! or just pulled up from the emulator module into the server module which is shared.
| // DO NOT REORDER/REMOVE | ||
| Unknown, Removed, InvalidParameters, NoPermission; | ||
| // DO NOT REORDER/REMOVE — ordinals are sent over the binder wire. Append new codes only. | ||
| Unknown, Removed, InvalidParameters, NoPermission, InvalidToken; |
There was a problem hiding this comment.
Should this just be sent in some other format over the wire to prevent that possible bug?
There was a problem hiding this comment.
I'm probably unnecessarily allergic to using strings for this, but there's really no reason this should ever change. I think the easy way to safeguard is to just have a unit test that verifies hardcoded ordinals.
| insertAtCursor(buildString { appendCodePoint(code) }) | ||
| } | ||
|
|
||
| override fun onSubmitWord(word: CharSequence) { |
There was a problem hiding this comment.
Is this something that was enabled by swiping in the keyboard upgrade? If so, why is this necessary if swiping is currently not supported in tools keyboards?
There was a problem hiding this comment.
It technically is supported! We just don't have a default model yet. And in this case, there are likely going to be other layouts that need to submit multiple chars at once.
|
|
||
| private var dismissJob: Job? = null | ||
|
|
||
| /** Show [modal] over the UI, replacing any current modal and restarting the dismiss timer. */ |
There was a problem hiding this comment.
So in other words there's no modal stack state? ie if Modal A has a dismiss duration of 5 seconds, and Modal B is 1 second -- after Modal B appears over Modal A and then is removed 1 second later, Modal A will be cleared early? Not that it really matters I guess.
There was a problem hiding this comment.
That's a very good point and worth considering actually, but so far these are explicitly meant to replace each other. conceptually they take up the same "space"
0x09f
left a comment
There was a problem hiding this comment.
This all seems fine to me! I'm definitely a Kotlin noob though so worth getting another set of 👀
|
|
||
| override fun onKeyUp(keyCode: Int, event: KeyEvent): Boolean { | ||
| return if (keyCode.isSystemKeyCode) { | ||
| super.onKeyDown(keyCode, event) |
There was a problem hiding this comment.
| super.onKeyDown(keyCode, event) | |
| super.onKeyUp(keyCode, event) |
| */ | ||
| var permissionActivity: Class<out Activity>? = null | ||
|
|
||
| var rootActivity: Class<out Activity>? = null |
There was a problem hiding this comment.
I couldn't find where this field is used
|
Looks great overall! Some review and LP3 testing notes:
|
drag listener already catches tap events
Reminder that if you are testing new features, they might not be available in LightOS yet. As of posting this PR, LightOS 568 is the latest for LPIII, and that will not yet be able to handle button presses forwarded from your tools