Skip to content

Extract enums from KeyboardId#2252

Draft
devycarol wants to merge 7 commits intoHeliBorg:mainfrom
devycarol:clean-keyboard-state
Draft

Extract enums from KeyboardId#2252
devycarol wants to merge 7 commits intoHeliBorg:mainfrom
devycarol:clean-keyboard-state

Conversation

@devycarol
Copy link
Contributor

I sat down to work on #2250 and realized I couldn't proceed before extracting the constant series from KeyboardId into enums. So that's what I've done.

I deleted the parallel XML enums since they're already out of date and don't seem to be used for anything? Let me know if this is a bad idea.

This one wound up being less commit-organized than the last one, apologies for that.

ALPHABET_AUTOMATIC_SHIFTED(R.string.spoken_description_mode_alpha),
ALPHABET_MANUAL_SHIFTED(R.string.spoken_description_shiftmode_on),
ALPHABET_SHIFT_LOCKED(R.string.spoken_description_shiftmode_locked),
ALPHABET_SHIFT_LOCK_SHIFTED(R.string.spoken_description_shiftmode_locked), // todo: what?
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean with the comment? That there is no separate string available?

val res = mKeyboardView.resources
val modeText = res.getString(keyboard.mId.mode.contentDescription)
val text = res.getString(R.string.announce_keyboard_mode, modeText)
// TODO: this is saying "Showing showing (text) keyboard"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did it always say "showing showing", or is this new?
How do I actually test this? Looks like I need "accessibility" enabled, but the accessibility settings on my phone don't have a generic on/off switch...

break;
case KeyCode.SEND_INTENT_ONE, KeyCode.SEND_INTENT_TWO, KeyCode.SEND_INTENT_THREE:
IntentUtils.handleSendIntentKey(mLatinIME, event.getKeyCode());
mLatinIME.requestHideSelf(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why add this? Even if it makes sense, it should not be in a completely unrelated commit / PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This catches me by surprise, I don't remember anything like this.

I need to look a lot closer at this patch and see if anything else fishy is going on. My best guess is it's a fragment from some incorrect merge/rebase handling on my part.

Apologies.

@Helium314
Copy link
Collaborator

No issues with the enum changes, but I'm not happy with the style changes in here. I think we discussed a somewhat similar style thing already in a previous PR...

My main issue is consistency (AOSP style might be pointless, but it's consistent). Now we mostly have the m prefix for public fields in java, except where we don't. We usually have a final, but in some files not. And I don't even see a pattern when the type is used, and when it's var.
I'm not opposed to such changes, but if they are useful / convenient for some reason, they should be done in all of the java code.
Btw is there some tooling support, ideally something that could force or warn about such style in Android Studio?

@eranl
Copy link
Contributor

eranl commented Feb 22, 2026

We usually have a final, but in some files not. And I don't even see a pattern when the type is used, and when it's var. I'm not opposed to such changes, but if they are useful / convenient for some reason, they should be done in all of the java code.

See #2249.

Btw is there some tooling support, ideally something that could force or warn about such style in Android Studio?

Sure. See Java 10 inspections for var, Java > Code style issues for final. You can get warnings in any direction you want.

@devycarol devycarol marked this pull request as draft February 24, 2026 09:30
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.

3 participants