ADFA-3153 | Introduce modular Code On the Go shortcut system#1083
Merged
ADFA-3153 | Introduce modular Code On the Go shortcut system#1083
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a keyboard shortcut system and integrates it into the app; introduces ShortcutManager, definitions, groups, and execution plumbing; wires shortcut dispatch into MainActivity and EditorHandlerActivity; replaces MainScreenAction with ActionItem-based registry and adds main-screen actions and utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Activity as MainActivity/<br/>EditorHandlerActivity
participant SM as ShortcutManager
participant Catalog as ShortcutCatalog
participant Exec as ShortcutExecutionContext
participant IdeActions as IdeShortcutActions
participant Registry as ActionsRegistry
participant Action as ActionItem
User->>Activity: press key
Activity->>Activity: dispatchKeyEvent(event)
Activity->>SM: dispatch(event, context, focusView, executionContext, hasModal)
SM->>Catalog: all(context)
Catalog-->>SM: definitions
SM->>SM: findMatchingShortcut(event, activeContexts)
SM->>Exec: execute matched definition
Exec->>IdeActions: execute(actionId)
IdeActions->>Registry: findActionById(actionId)
Registry-->>IdeActions: ActionItem
IdeActions->>Action: prepare(data) + execAction(data)
Action-->>IdeActions: result
IdeActions-->>Exec: success/failure
Exec-->>SM: propagate result
SM-->>Activity: handled?
alt handled
Activity-->>User: action executed
else not handled
Activity->>Activity: super.dispatchKeyEvent()
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutGroupProvider.kt (1)
13-18: Cache group instances instead of recreating them on everyall()call.These groups are stateless, so rebuilding the list each call adds unnecessary allocations.
♻️ Proposed refactor
class ShortcutGroupProvider { - fun all(): List<ShortcutGroup> = listOf( - ProjectsAndSolutionsGroup(), - WindowsAndDisplayGroup(), - FileManagementGroup(), - SearchAndReplaceGroup(), - ) + private val groups: List<ShortcutGroup> = listOf( + ProjectsAndSolutionsGroup(), + WindowsAndDisplayGroup(), + FileManagementGroup(), + SearchAndReplaceGroup(), + ) + + fun all(): List<ShortcutGroup> = groups }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutGroupProvider.kt` around lines 13 - 18, The all() function currently constructs new ShortcutGroup instances on every call causing needless allocations; change ShortcutGroupProvider to create and store a single immutable list of those stateless groups (e.g., a private val like cachedGroups = listOf(ProjectsAndSolutionsGroup(), WindowsAndDisplayGroup(), FileManagementGroup(), SearchAndReplaceGroup())) and have fun all(): List<ShortcutGroup> return that cached list instead of rebuilding it each time.app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt (1)
145-151: Avoid recreating execution objects on every key event.
editorShortcutExecutionContext()allocates newShortcutExecutionContextandIdeShortcutActionsfor each dispatch; this can be a cached property since the provider lambda already resolves freshActionData.♻️ Proposed refactor
- private fun editorShortcutExecutionContext(): ShortcutExecutionContext { - return ShortcutExecutionContext( - ideShortcutActions = IdeShortcutActions { - createToolbarActionData() - }, - ) - } + private val editorShortcutExecutionContext by lazy { + ShortcutExecutionContext( + ideShortcutActions = IdeShortcutActions { + createToolbarActionData() + }, + ) + }- executionContext = editorShortcutExecutionContext(), + executionContext = editorShortcutExecutionContext,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt` around lines 145 - 151, The method editorShortcutExecutionContext() currently constructs a new ShortcutExecutionContext and IdeShortcutActions on every call; change this to a cached property (e.g., a private val or lazy-initialized field) that holds a single ShortcutExecutionContext instance so it isn't reallocated per key event, while keeping the IdeShortcutActions provider lambda calling createToolbarActionData() unchanged; update usages to reference the cached property instead of calling editorShortcutExecutionContext() repeatedly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/itsaky/androidide/shortcuts/groups/FileManagementGroup.kt`:
- Around line 16-27: The shortcut ID ShortcutDefinitionIds.SAVE_FILE is
misleading because SaveFileAction.ID ("ide.editor.files.saveAll") and its
implementation (context.saveAllResult(), UI string "all_saved") save all open
files; either rename the constant to SAVE_ALL_FILES and update all references to
use ShortcutDefinitionIds.SAVE_ALL_FILES and the ShortcutDefinition id, or keep
the ID but update the shortcut title/tooltip/documentation to explicitly say
"Save all files" (and adjust any string resources if needed) so the Ctrl+S
binding and SaveFileAction.ID consistently communicate that it saves all open
files.
In
`@app/src/main/java/com/itsaky/androidide/shortcuts/groups/SearchAndReplaceGroup.kt`:
- Around line 16-39: The two ShortcutDefinition entries for
ShortcutDefinitionIds.FIND_IN_PROJECT and ShortcutDefinitionIds.FIND_IN_FILE
have their KeyShortcut bindings inverted; change the FIND_IN_PROJECT entry to
use KeyShortcut.ctrlShift(KeyEvent.KEYCODE_F) and change the FIND_IN_FILE entry
to use KeyShortcut.ctrl(KeyEvent.KEYCODE_F) so Ctrl+F triggers
FindInFileAction.ID and Ctrl+Shift+F triggers FindInProjectAction.ID, keeping
titles and actionId values (FindInProjectAction.ID, FindInFileAction.ID) as-is.
In `@app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutManager.kt`:
- Around line 48-63: The priority calculation in findMatchingShortcut is using
definition.contexts (which may include inactive contexts) when calling
priorityScore; update the scoring to only consider the intersection of
definition.contexts with the provided activeContexts (e.g., compute active =
definition.contexts.filter { it in activeContexts } or
.intersect(activeContexts)) and then use active.maxOfOrNull { shortcutContext ->
priorityScore(shortcutContext) } ?: Int.MIN_VALUE so inactive contexts (like
MODAL when not active) do not inflate a shortcut's priority.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt`:
- Around line 145-151: The method editorShortcutExecutionContext() currently
constructs a new ShortcutExecutionContext and IdeShortcutActions on every call;
change this to a cached property (e.g., a private val or lazy-initialized field)
that holds a single ShortcutExecutionContext instance so it isn't reallocated
per key event, while keeping the IdeShortcutActions provider lambda calling
createToolbarActionData() unchanged; update usages to reference the cached
property instead of calling editorShortcutExecutionContext() repeatedly.
In `@app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutGroupProvider.kt`:
- Around line 13-18: The all() function currently constructs new ShortcutGroup
instances on every call causing needless allocations; change
ShortcutGroupProvider to create and store a single immutable list of those
stateless groups (e.g., a private val like cachedGroups =
listOf(ProjectsAndSolutionsGroup(), WindowsAndDisplayGroup(),
FileManagementGroup(), SearchAndReplaceGroup())) and have fun all():
List<ShortcutGroup> return that cached list instead of rebuilding it each time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c18d95fe-bef9-4470-90a5-045746c5c72c
📒 Files selected for processing (22)
app/src/main/java/com/itsaky/androidide/actions/file/CloseFileAction.ktapp/src/main/java/com/itsaky/androidide/actions/sidebar/PreferencesSidebarAction.ktapp/src/main/java/com/itsaky/androidide/activities/MainActivity.ktapp/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.ktapp/src/main/java/com/itsaky/androidide/shortcuts/IdeShortcutActions.ktapp/src/main/java/com/itsaky/androidide/shortcuts/KeyShortcut.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutActionIds.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutCatalog.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutCategory.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutContext.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutDefinition.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutExecutionContext.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutGroupProvider.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutManager.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/FileManagementGroup.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/ProjectsAndSolutionsGroup.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/SearchAndReplaceGroup.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/ShortcutDefinitionIds.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/ShortcutGroup.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/WindowsAndDisplayGroup.ktapp/src/main/java/com/itsaky/androidide/utils/FragmentUtils.ktresources/src/main/res/values/strings.xml
app/src/main/java/com/itsaky/androidide/shortcuts/groups/FileManagementGroup.kt
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/shortcuts/groups/SearchAndReplaceGroup.kt
Show resolved
Hide resolved
jatezzz
force-pushed
the
feat/ADFA-3153-ide-shortcut-system
branch
from
f0fc199 to
a4b67d4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/utils/FragmentUtils.kt (1)
6-17: Consider extracting shared fragment-tree traversal logic.
hasVisibleDialog()anddismissTopDialog()duplicate near-identical DFS traversal. A shared private walker/helper would reduce drift between detection and dismissal behavior.Also applies to: 19-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/utils/FragmentUtils.kt` around lines 6 - 17, Both hasVisibleDialog() and dismissTopDialog() implement the same DFS over FragmentManager fragments; extract a single private helper (e.g., FragmentManager.walkFragments / FragmentManager.findInFragments or FragmentUtils.traverseFragmentTree) that performs recursive traversal and accepts a lambda predicate/action to avoid duplication, then rewrite hasVisibleDialog() to call the helper with a predicate checking "fragment is DialogFragment && fragment.dialog?.isShowing == true" and rewrite dismissTopDialog() to call the same helper to locate and dismiss the dialog fragment; ensure the helper uses fragment.childFragmentManager recursion and returns early when the predicate/action signals a match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/utils/FragmentUtils.kt`:
- Around line 12-13: Guard access to fragment.childFragmentManager by checking
fragment.isAdded before recursing into child fragments: wherever you call
fragment.childFragmentManager.hasVisibleDialog() (both occurrences that
currently access childFragmentManager unconditionally), first verify
fragment.isAdded and only then call childFragmentManager.hasVisibleDialog();
this prevents IllegalStateException for unattached fragments while preserving
the existing recursion/return logic.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/utils/FragmentUtils.kt`:
- Around line 6-17: Both hasVisibleDialog() and dismissTopDialog() implement the
same DFS over FragmentManager fragments; extract a single private helper (e.g.,
FragmentManager.walkFragments / FragmentManager.findInFragments or
FragmentUtils.traverseFragmentTree) that performs recursive traversal and
accepts a lambda predicate/action to avoid duplication, then rewrite
hasVisibleDialog() to call the helper with a predicate checking "fragment is
DialogFragment && fragment.dialog?.isShowing == true" and rewrite
dismissTopDialog() to call the same helper to locate and dismiss the dialog
fragment; ensure the helper uses fragment.childFragmentManager recursion and
returns early when the predicate/action signals a match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad1f65f0-ec77-4c2b-b2aa-a82ad769f00e
📒 Files selected for processing (22)
app/src/main/java/com/itsaky/androidide/actions/file/CloseFileAction.ktapp/src/main/java/com/itsaky/androidide/actions/sidebar/PreferencesSidebarAction.ktapp/src/main/java/com/itsaky/androidide/activities/MainActivity.ktapp/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.ktapp/src/main/java/com/itsaky/androidide/shortcuts/IdeShortcutActions.ktapp/src/main/java/com/itsaky/androidide/shortcuts/KeyShortcut.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutActionIds.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutCatalog.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutCategory.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutContext.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutDefinition.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutExecutionContext.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutGroupProvider.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutManager.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/FileManagementGroup.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/ProjectsAndSolutionsGroup.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/SearchAndReplaceGroup.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/ShortcutDefinitionIds.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/ShortcutGroup.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/WindowsAndDisplayGroup.ktapp/src/main/java/com/itsaky/androidide/utils/FragmentUtils.ktresources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (19)
- app/src/main/java/com/itsaky/androidide/shortcuts/groups/ShortcutGroup.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/groups/ProjectsAndSolutionsGroup.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutManager.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutDefinition.kt
- resources/src/main/res/values/strings.xml
- app/src/main/java/com/itsaky/androidide/shortcuts/groups/ShortcutDefinitionIds.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/IdeShortcutActions.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutCategory.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutGroupProvider.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutExecutionContext.kt
- app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/groups/SearchAndReplaceGroup.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutContext.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutActionIds.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/KeyShortcut.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/groups/FileManagementGroup.kt
- app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt
- app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutCatalog.kt
- app/src/main/java/com/itsaky/androidide/actions/sidebar/PreferencesSidebarAction.kt
itsaky-adfa
requested changes
Mar 17, 2026
app/src/main/java/com/itsaky/androidide/shortcuts/groups/SearchAndReplaceGroup.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/shortcuts/IdeShortcutActions.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/shortcuts/KeyShortcut.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/shortcuts/IdeShortcutActions.kt
Outdated
Show resolved
Hide resolved
davidschachterADFA
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
app/src/main/java/com/itsaky/androidide/actions/main/DonateAction.kt (1)
29-35: Collapse the dead visibility branch.Line 34 hides the action unconditionally, so the null-context check on Lines 31-33 never changes behavior. Removing it would make the temporary disablement clearer.
✂️ Suggested cleanup
override fun prepare(data: ActionData) { super.prepare(data) - if (data.get(Context::class.java) == null) { - markInvisible() - } markInvisible() // Until we allow donations }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/actions/main/DonateAction.kt` around lines 29 - 35, In DonateAction.prepare(ActionData) remove the dead conditional that checks data.get(Context::class.java) for null since markInvisible() is called unconditionally; delete the if block (the null-context check and its markInvisible() call) so the method only calls super.prepare(data) and the single temporary markInvisible(), keeping DonateAction and prepare as the points to edit.app/src/main/java/com/itsaky/androidide/fragments/MainFragment.kt (1)
54-58: Avoid hard-wiringDefaultActionsRegistryinto the fragment.This screen already depends on
ActionsRegistry, but the concrete type check means any alternative implementation or test double turns clicks into no-ops. Moving execution behind the registry API, or a dedicated executor, would keep the new shortcut/action flow extensible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/fragments/MainFragment.kt` around lines 54 - 58, The fragment currently hard-checks registry is a DefaultActionsRegistry in the onClick handler which prevents other ActionsRegistry implementations from executing actions; remove the DefaultActionsRegistry type check and invoke the execute method via the registry interface (or inject a small ActionExecutor and call its execute(action, actionData) from the onClick lambda). If the ActionsRegistry interface lacks an executeAction(ActionItem, actionData) method, add that method to the interface (or add the new ActionExecutor abstraction) and update callers to use the interface method rather than casting to DefaultActionsRegistry; keep references to onClick, ActionItem, registry, executeAction and actionData to locate the change.app/src/main/java/com/itsaky/androidide/actions/main/OpenProjectAction.kt (1)
13-43: Consider extracting a shared base for main-screen navigation actions.This class duplicates the same scaffolding pattern used in
CreateProjectActionandCloneRepositoryAction(metadata setup +MainActivitygating + safe execution). A small abstract base would reduce drift and keep behavior consistent.Refactor sketch
+abstract class MainScreenNavigationAction( + context: Context, + final override val id: String, + labelRes: Int, + iconRes: Int, + final override val order: Int, +) : ActionItem { + final override var label: String = context.getString(labelRes) + final override var visible: Boolean = true + final override var enabled: Boolean = true + final override var icon: Drawable? = ContextCompat.getDrawable(context, iconRes) + final override var requiresUIThread: Boolean = true + final override var location: ActionItem.Location = ActionItem.Location.MAIN_SCREEN + + override fun prepare(data: ActionData) { + super.prepare(data) + if (data.get(Context::class.java) !is MainActivity) { + markInvisible() + } + } +}-class OpenProjectAction(context: Context) : ActionItem { +class OpenProjectAction(context: Context) : MainScreenNavigationAction( + context = context, + id = ID, + labelRes = R.string.msg_open_existing_project, + iconRes = R.drawable.ic_folder, + order = 1, +) { companion object { const val ID = "ide.main.openProject" } - ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/actions/main/OpenProjectAction.kt` around lines 13 - 43, Extract a small abstract base (e.g., MainScreenAction or MainActivityAction) that implements the common ActionItem scaffolding used by OpenProjectAction, CreateProjectAction, and CloneRepositoryAction: move shared properties (label, visible, enabled, icon, requiresUIThread, location, order) and the MainActivity gating logic from prepare() into the base, provide a protected abstract suspend fun perform(main: MainActivity): Any (or similar) that concrete actions implement, and implement execAction() in the base to safely cast data.get(Context::class.java) to MainActivity and call perform(), then update OpenProjectAction (and the other two classes) to extend this base and implement perform() instead of duplicating prepare()/execAction().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/actions/main/DocsAction.kt`:
- Around line 45-47: The Intent currently sets CONTENT_TITLE_KEY to
context.getString(string.back_to_cogo) which will label the HelpActivity with
the back-navigation text; change the extra so CONTENT_TITLE_KEY uses the
docs/help title resource instead (e.g., context.getString(string.docs) or the
appropriate R.string.help label) so HelpActivity's action bar shows the page
title; update the putExtra call that references CONTENT_TITLE_KEY in the
Intent(...) block that launches HelpActivity.
In `@app/src/main/java/com/itsaky/androidide/adapters/MainActionsListAdapter.kt`:
- Around line 54-65: The button's enabled state isn't bound to action.enabled,
leaving a control that looks active but ignores clicks; update the view binding
(binding.root) to reflect action.enabled (e.g., set binding.root.isEnabled =
action.enabled and optionally binding.root.isClickable = action.enabled) so the
UI is visually and interactively disabled when action.enabled is false; make
this change near the existing setup code that assigns text, icon,
contentDescription and the listeners in MainActionsListAdapter (where
action.label, action.icon and the click/long-click handlers are configured).
In `@app/src/main/java/com/itsaky/androidide/fragments/MainFragment.kt`:
- Around line 45-53: MainFragment is taking a one-time snapshot of
ActionsRegistry.getInstance().getActions(ActionItem.Location.MAIN_SCREEN).values
which can become stale because MainActivity re-registers and clears MAIN_SCREEN
actions in onResume(); update MainFragment to refresh its action list after the
final registration point (e.g., move the getActions(...) call into
onResume()/onStart or add a registry change callback) so adapters always use the
live ActionItem instances; reference ActionsRegistry.getInstance(),
getActions(ActionItem.Location.MAIN_SCREEN), MainFragment adapter setup, and
MainActivity.onResume() when implementing the refresh or listener approach.
In `@app/src/main/java/com/itsaky/androidide/shortcuts/IdeShortcutActions.kt`:
- Around line 29-30: The code currently returns early when actionsRegistry isn't
a DefaultActionsRegistry or when findActionById(...) returns null, which
prevents the fallback path from running; instead, remove those early returns and
let the method continue to the fallback handling: treat registry as nullable
(keep "val registry = actionsRegistry as? DefaultActionsRegistry" without
returning) and if "val action = findActionById(actionsRegistry, actionId)" is
null do not return false but fall through so the existing fallback execution in
ShortcutManager.dispatch can run; reference DefaultActionsRegistry,
actionsRegistry, findActionById, actionId and the dispatch/fallback flow when
updating the control flow.
- Around line 31-33: After calling action.prepare(data) the code only checks
action.enabled which lets actions that were marked invisible during prepare()
still run; update the guard to also respect visibility by checking
action.visible (or equivalent) after prepare and return false if the action is
not visible before calling registry.executeAction(action, data). Locate the call
sequence action.prepare(data), action.enabled and registry.executeAction(action,
data) and add a visibility check (ensure you use the action.visible/isVisible
property or method used elsewhere, e.g., DonateAction.markInvisible()) so hidden
actions are not executed via shortcuts.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/actions/main/DonateAction.kt`:
- Around line 29-35: In DonateAction.prepare(ActionData) remove the dead
conditional that checks data.get(Context::class.java) for null since
markInvisible() is called unconditionally; delete the if block (the null-context
check and its markInvisible() call) so the method only calls super.prepare(data)
and the single temporary markInvisible(), keeping DonateAction and prepare as
the points to edit.
In `@app/src/main/java/com/itsaky/androidide/actions/main/OpenProjectAction.kt`:
- Around line 13-43: Extract a small abstract base (e.g., MainScreenAction or
MainActivityAction) that implements the common ActionItem scaffolding used by
OpenProjectAction, CreateProjectAction, and CloneRepositoryAction: move shared
properties (label, visible, enabled, icon, requiresUIThread, location, order)
and the MainActivity gating logic from prepare() into the base, provide a
protected abstract suspend fun perform(main: MainActivity): Any (or similar)
that concrete actions implement, and implement execAction() in the base to
safely cast data.get(Context::class.java) to MainActivity and call perform(),
then update OpenProjectAction (and the other two classes) to extend this base
and implement perform() instead of duplicating prepare()/execAction().
In `@app/src/main/java/com/itsaky/androidide/fragments/MainFragment.kt`:
- Around line 54-58: The fragment currently hard-checks registry is a
DefaultActionsRegistry in the onClick handler which prevents other
ActionsRegistry implementations from executing actions; remove the
DefaultActionsRegistry type check and invoke the execute method via the registry
interface (or inject a small ActionExecutor and call its execute(action,
actionData) from the onClick lambda). If the ActionsRegistry interface lacks an
executeAction(ActionItem, actionData) method, add that method to the interface
(or add the new ActionExecutor abstraction) and update callers to use the
interface method rather than casting to DefaultActionsRegistry; keep references
to onClick, ActionItem, registry, executeAction and actionData to locate the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb52302e-048e-4763-b29a-09ed47bbdd52
📒 Files selected for processing (21)
actions/src/main/java/com/itsaky/androidide/actions/ActionItem.ktapp/src/main/java/com/itsaky/androidide/actions/main/CloneRepositoryAction.ktapp/src/main/java/com/itsaky/androidide/actions/main/CreateProjectAction.ktapp/src/main/java/com/itsaky/androidide/actions/main/DeleteProjectAction.ktapp/src/main/java/com/itsaky/androidide/actions/main/DocsAction.ktapp/src/main/java/com/itsaky/androidide/actions/main/DonateAction.ktapp/src/main/java/com/itsaky/androidide/actions/main/OpenProjectAction.ktapp/src/main/java/com/itsaky/androidide/actions/main/OpenTerminalAction.ktapp/src/main/java/com/itsaky/androidide/actions/main/PreferencesAction.ktapp/src/main/java/com/itsaky/androidide/activities/MainActivity.ktapp/src/main/java/com/itsaky/androidide/adapters/MainActionsListAdapter.ktapp/src/main/java/com/itsaky/androidide/fragments/MainFragment.ktapp/src/main/java/com/itsaky/androidide/models/MainScreenAction.ktapp/src/main/java/com/itsaky/androidide/shortcuts/IdeShortcutActions.ktapp/src/main/java/com/itsaky/androidide/shortcuts/KeyShortcut.ktapp/src/main/java/com/itsaky/androidide/shortcuts/ShortcutManager.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/ProjectsAndSolutionsGroup.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/SearchAndReplaceGroup.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/ShortcutDefinitionIds.ktapp/src/main/java/com/itsaky/androidide/shortcuts/groups/WindowsAndDisplayGroup.ktapp/src/main/java/com/itsaky/androidide/utils/MainScreenActions.kt
💤 Files with no reviewable changes (1)
- app/src/main/java/com/itsaky/androidide/models/MainScreenAction.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt
app/src/main/java/com/itsaky/androidide/adapters/MainActionsListAdapter.kt
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/fragments/MainFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/shortcuts/IdeShortcutActions.kt
Outdated
Show resolved
Hide resolved
Collaborator
|
Please divide this into 2 smaller PRs - thanks |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/fragments/MainFragment.kt (1)
108-111: Optional: extract the split index into a named constant.Using a named constant for
3will make the portrait/landscape action partition easier to maintain.♻️ Suggested tweak
class MainFragment : BaseFragment() { + private companion object { + const val LANDSCAPE_LEFT_ACTION_COUNT = 3 + } @@ - val leftActions = if (binding!!.actionsRight != null) actions.take(3) else actions + val leftActions = if (binding!!.actionsRight != null) { + actions.take(LANDSCAPE_LEFT_ACTION_COUNT) + } else actions binding!!.actions.adapter = MainActionsListAdapter(leftActions, onClick, onLongClick) - binding!!.actionsRight?.adapter = MainActionsListAdapter(actions.drop(3), onClick, onLongClick) + binding!!.actionsRight?.adapter = MainActionsListAdapter( + actions.drop(LANDSCAPE_LEFT_ACTION_COUNT), + onClick, + onLongClick + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/fragments/MainFragment.kt` around lines 108 - 111, The hard-coded split index "3" used to partition actions (seen in actions.take(3) and actions.drop(3) when creating leftActions and setting adapters for binding!!.actions and binding!!.actionsRight) should be extracted to a named constant (e.g., ACTIONS_SPLIT_INDEX) and used in place of both literals; update the leftActions assignment and the adapter for actionsRight to reference ACTIONS_SPLIT_INDEX and keep MainActionsListAdapter creation unchanged so the partition logic is centralized and easier to adjust.app/src/main/java/com/itsaky/androidide/adapters/MainActionsListAdapter.kt (1)
63-65: Consider not consuming long-click when no handler is provided.Defaulting to
truehere swallows long-press events even when the adapter has no long-click behavior.♻️ Suggested tweak
setOnLongClickListener { - onLongClick?.invoke(action, it) ?: true + onLongClick?.invoke(action, it) ?: false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/adapters/MainActionsListAdapter.kt` around lines 63 - 65, The long-click listener currently always consumes the event by returning true when no handler exists; change the return to not consume the event by using the actual handler result or false when absent (replace the Elvis default from true to false), i.e. update the setOnLongClickListener lambda that calls onLongClick?.invoke(action, it) to return false when onLongClick is null so long-presses bubble through.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/adapters/MainActionsListAdapter.kt`:
- Around line 63-65: The long-click listener currently always consumes the event
by returning true when no handler exists; change the return to not consume the
event by using the actual handler result or false when absent (replace the Elvis
default from true to false), i.e. update the setOnLongClickListener lambda that
calls onLongClick?.invoke(action, it) to return false when onLongClick is null
so long-presses bubble through.
In `@app/src/main/java/com/itsaky/androidide/fragments/MainFragment.kt`:
- Around line 108-111: The hard-coded split index "3" used to partition actions
(seen in actions.take(3) and actions.drop(3) when creating leftActions and
setting adapters for binding!!.actions and binding!!.actionsRight) should be
extracted to a named constant (e.g., ACTIONS_SPLIT_INDEX) and used in place of
both literals; update the leftActions assignment and the adapter for
actionsRight to reference ACTIONS_SPLIT_INDEX and keep MainActionsListAdapter
creation unchanged so the partition logic is centralized and easier to adjust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eaedb67e-0f6a-4ffa-85c9-85a41c1385f4
📒 Files selected for processing (2)
app/src/main/java/com/itsaky/androidide/adapters/MainActionsListAdapter.ktapp/src/main/java/com/itsaky/androidide/fragments/MainFragment.kt
jatezzz
force-pushed
the
feat/ADFA-3153-ide-shortcut-system
branch
from
d31f305 to
a4b67d4
Compare
Collaborator
Author
|
@itsaky-adfa Thanks for your comments, you were absolutely right. As @hal-eisen-adfa requested, the PR was split into two to maintain better reviewability. The changes have been addressed in #1091. Could you please take a look? |
itsaky-adfa
requested changes
Mar 19, 2026
app/src/main/java/com/itsaky/androidide/shortcuts/groups/SearchAndReplaceGroup.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/shortcuts/KeyShortcut.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/shortcuts/ShortcutManager.kt
Outdated
Show resolved
Hide resolved
itsaky-adfa
requested changes
Mar 19, 2026
app/src/main/java/com/itsaky/androidide/shortcuts/groups/WindowsAndDisplayGroup.kt
Outdated
Show resolved
Hide resolved
itsaky-adfa
approved these changes
Mar 19, 2026
… system add grouped shortcuts and editor dispatch integration
refactor(shortcuts): rename saveFile to saveAllFiles for consistency
* fix(main): refresh main screen actions on resume refactor(shortcuts): route main screen actions through registry and update bindings * refactor: migrate shortcut actions logging to SLF4J and improve error visibility Add logs for missing action data, registry mismatch, and unresolved action IDs to avoid silent failures
jatezzz
force-pushed
the
feat/ADFA-3153-ide-shortcut-system
branch
from
3dd82c7 to
1e9747f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces a modular keyboard shortcut system for COGO.
The implementation organizes shortcuts into logical groups, centralizes their registration in a catalog, and adds a structured execution layer capable of resolving actions through the IDE action registry with built-in fallbacks.
The editor activity is now integrated with the shortcut dispatcher, enabling context-aware shortcut handling (e.g., editor, main screen, modal dialogs).
Details
Key improvements included in this PR:
Introduced ShortcutGroup architecture to organize shortcuts by functional areas.
Added grouped shortcut implementations:
ProjectsAndSolutionsGroupWindowsAndDisplayGroupSearchAndReplaceGroupFileManagementGroup(referenced in provider)Implemented ShortcutCatalog for centralized aggregation of shortcuts.
Added ShortcutCategory and ShortcutContext enums for classification and contextual filtering.
Implemented ShortcutGroupProvider to register all available groups.
Introduced IdeShortcutActions executor to:
ActionsRegistryAdded ShortcutActionIds as a central registry for built-in COGO shortcut actions.
Added ShortcutExecutionContext abstraction for passing execution dependencies.
Integrated shortcut dispatching into
EditorHandlerActivityviadispatchKeyEvent.Added support for handling modal dismissal using
ESC.This structure enables future shortcut additions without modifying core shortcut dispatch logic.
document_5019473586721654554.1.mp4
Ticket
ADFA-3153
Observation
The shortcut system is designed to be extensible by introducing new
ShortcutGroupimplementations without modifying the central catalog logic.Fallback execution ensures that essential COGO behaviors remain functional even if an action is not registered in the
ActionsRegistry.