Skip to content

Conversation

NaMinhyeok
Copy link
Contributor

Fixes a bug where the ESC key does not properly exit insert mode in read-only files.
This happens when users attempt insert commands (i, a, o) in read-only files (e.g., .http requests).
Commands appear to succeed but leave the editor in an inconsistent state.

Root Cause

  1. Insert commands were moving the cursor before checking if the file was writable.
  2. This caused an inconsistent state: cursor moved, but insert mode was never properly active.
  3. As a result, the ESC handler became confused, leaving the editor stuck.

Solution

I considered two approaches:

Approach 1: Fix ESC behavior only

  • Change InsertExitModeAction type from INSERTMODE_CHANGE
  • Minimal change
  • Still allows entering insert mode

Approach 2: Prevent insert mode entry in read-only files (chosen)

  • Add read-only checks before cursor movement in insert commands
  • Prevents entering insert mode

I'd appreciate feedback on whether this approach aligns with IdeaVim’s overall design philosophy.
Specifically:

  • Is preventing insert mode in read-only editors the right direction, or should ESC behavior alone be fixed?
  • Are there any edge cases or test scenarios (e.g., visual block insert, macros, completion popups) that I might have missed?

Happy to add more test coverage or adjust the approach based on your feedback. Thanks in advance for reviewing!

Fixes VIM-3541 this issue has ideaVim-bounty tag

@wxh06
Copy link
Contributor

wxh06 commented Aug 26, 2025

Personally, I consider it reasonable for the "Clear Read-Only Status" dialog to pop up when attempting to enter insert mode, rather than preventing the user from entering it.
You may be interested in 83e5470, though it does not appear to function as intended.

Furthermore, checking solely upon entering insert mode is insufficient to resolve the issue entirely. The user could switch to another read-only file while in insert mode on a writable file, causing the mode to persist (as described in VIM-3563).

@NaMinhyeok
Copy link
Contributor Author

@wxh06
Thanks for the feedback! I’ll look into the following points you mentioned
I’ll experiment with these scenarios and adjust the implementation accordingly.

@NaMinhyeok
Copy link
Contributor Author

Thank you for the review, and I apologize for overlooking the test case involving a manually made read-only file.
Previously, I only tested with files that are inherently non-writable, such as Array.kt.

I would like to ask for clarification on a few points. As I understand, the “Clear Read-Only Status” dialog is already implemented. However, it appears that this dialog does not appear immediately upon entering insert mode, but rather after a subsequent action.
For example, with commands such as o or cw, the dialog appears right away. By contrast, after commands like i or a, the dialog is only shown after a later action such as pressing ESC. I plan to investigate this behavior further.

In this context, I see two possible scenarios:

  1. Writable file made read-only via “Make Read-Only”

    • In this case, should pressing ESC restore the writable state, or should the file remain read-only until the user explicitly clears the status?
  2. Files that are inherently non-writable

    • Since these files cannot be made writable at all, would it be preferable to prevent entering insert mode entirely?
    • In such cases, should we still show the “Clear Read-Only Status” dialog, or would it be more consistent with the original issue to simply allow ESC to exit insert mode?
    • I believe this second case may be particularly important to clarify, as it directly impacts the user experience with non-editable files.

Additionally, regarding VIM-3563:
As far as I can tell, this issue appears to have already been resolved, since I have not been able to reproduce the problem.

Once I hear your thoughts on the scenarios above, I will take them into consideration, along with VIM-3563, to ensure the implementation avoids reintroducing similar issues.

Can you comment on this? @wxh06

@wxh06
Copy link
Contributor

wxh06 commented Aug 27, 2025

@NaMinhyeok Thank you for your efforts; there's no need to apologise for this, as discussion is welcome.

My apologies, I was mistaken about the behavior in VIM-3563 earlier. You were correct—it has been fully resolved. The scenario I attempted to describe earlier should be precisely what you mentioned: setting a writable file to read-only.

I have no particular expectations regarding the behaviour in IdeaVim. Might @AlexPl292 have some expectations on this?
For reference, Vim permits editing read-only files, though it displays a warning upon entering insert mode. Preventing any modifications to read-only files appears to be an IntelliJ platform behaviour, rendering Vim-like consistency entirely impossible. Consequently, there seems little justification for allowing users to enter insert mode within read-only files?

Personally, I consider the inability to exit insert mode within read-only files to be a bug in itself. The primary fix should ensure that users can exit insert mode regardless of how they inadvertently entered it.

@NaMinhyeok
Copy link
Contributor Author

Thanks for the thoughtful review.

I agree that because the IntelliJ Platform prevents actual edits in read-only files, full parity with Vim isn't always attainable.
Given that, for files that are truly non-writable, it seems most logical to block entry into insert mode entirely, as no changes can be made anyway.

Regarding the inconsistent dialog behavior I raised earlier (appearing immediately for o/cw but delayed for i/a), my current changes have resolved this to ensure consistent behavior.

This approach also solves the main concern you raised: being unable to exit insert mode.
If we block insert mode from the start, the problem of being stuck in insert mode will never happen.

However, if IdeaVim prefers to follow Vim's default of allowing entry into insert mode, I am happy to adapt the patch to ensure ESC always exits cleanly.

I’ll also wait for @AlexPl292’s comment.

@AlexPl292
Copy link
Member

Hello everyone, thank you for an interesting discussion to read.
First of all, I confirm that we'll be ready to give the bounty for the proper fix of VIM-3541.

Now, let's split this question into two:

Exiting the insert mode in read-only files

Even if we prohibit entering the insert mode in the read-only files, I believe we should properly support exiting the insert mode. The reason is that if the user appears in the insert mode by some magic reason, they should have the ability to exit it. Putting a read-only status in insert mode is a good example of such a magical reason.

The esc and InsertExitModeAction are writable for a reason. One of the reasons is described in the code comment of InsertExitModeAction. The second case I remember is that putting a number before entering insert mode will cause the inserted text to be repeated after pressing Esc. So, like 3ia<esc> will result in aaa.
However, lately we started to come to the conclusion that the mechanism of Command.Type is weird and obsolete. This is, by the way, a good example of it: 99% the user should not have a writing lock when pressing esc, but we're making it anyway for this 1%.
So, I'd consider a proper fix to be more complicated:

  • Switching InsertExitModeAction to OTHER_SELF_SYNCHRONIZED type, which currently means that the command handles read/write locks by itself.
  • Do not take any locks during the work of the ESC.
  • Unless we face the part of the code when we insert additional text (the cases described above). These places can be wrapped with runWriteAction. Also, before entering these places we can show a "Do you want to remove the read-only status?" and if the user doesn't remove this status, we just exit the insert mode without additional inserts.

Entering the insert mode in the read-only files

I've tried to enter insert mode for the help files in Vim, and it didn't allow me. So, I didn't see the case when we can enter the insert mode for the read-only files. But generally, this looks weird for me as the insert mode implies some kind of modification.

I think, it makes sence to show a dialog right on i and friends for files that can be switched to the writable. If the user agrees to make the file writable, it should remain writable as these changes are coming more from IJ rather then from IdeaVim and if they want to switch back to read-only, they can do it manually.
If the user disagrees with enabling writable status, we can leave the normal mode. And same for files that cannot be writable at all: just leave the normal mode and show some status-bar warning about it.

Thank you for your interest in the question! If I missed some questions, please let me know.

@NaMinhyeok
Copy link
Contributor Author

Thank you for the detailed review and guidance.

Based on your feedback, I've summarized my understanding of the desired behavior into two cases. Could you please confirm if this is correct?

Not Allow Writable Case (like Vim help files)

  • If a file cannot become writable at all, we should not allow entering insert mode.
  • When a user tries to enter insert mode, we just show a warning message with IJ behavior (like current behavior) and dont't allow insert mode
  • If the user somehow enters insert mode anyway, pressing ESC should always exit back to normal mode.

Writable Case (temporarily read-only files)

  • If a file can be writable, but is currently read-only, we should show the “Clear Read-Only Status” dialog when entering insert mode.
    • If the user agrees, make the file writable and stay in insert mode.
    • If the user refuses, return to normal mode.

Just to clarify, in the writable case, if the file becomes writable by confirming the dialog, pressing ESC should only return to normal mode — not revert the file back to read-only, right?

Additionally, I have reviewed and understood your proposal for the more complex but proper fix for the Esc key behavior, specifically regarding changing InsertExitModeAction to OTHER_SELF_SYNCHRONIZED and handling the write locks manually.

I will attempt to implement a solution based on this direction and will share my progress.

@wxh06
Copy link
Contributor

wxh06 commented Aug 29, 2025

@AlexPl292:
I've tried to enter insert mode for the help files in Vim, and it didn't allow me.

It is gratifying to learn that Vim also prevents users from entering insert mode, providing factual grounds for us to prohibit such actions (for instance, in decompiled files).

So, I didn't see the case when we can enter the insert mode for the read-only files.

The warning message I mentioned earlier appears in files that are editable but lack write permissions. Upon attempting to save, an additional error message indicating the file cannot be saved will appear.
I believe it is reasonable for IdeaVim to adapt this scenario by showing a ‘Clear Read-Only Status’ pop-up, as IntelliJ typically saves files during editing.

Attempt to enter insert mode in a 'readonly' file, which is editable but lack write permissions Attempt to save the file Attempt to enter insert mode in an unmodifiable file
W10 E45 E21

@AlexPl292
Copy link
Member

AlexPl292 commented Aug 29, 2025

@NaMinhyeok Yes, I confirm.

pressing ESC should only return to normal mode — not revert the file back to read-only, right?

Yes, I think not reverting the read-only is a better way.

Thank you!

@NaMinhyeok
Copy link
Contributor Author

NaMinhyeok commented Sep 1, 2025

@AlexPl292 Thank you for the guidance! I've implemented the solution following your suggestions:

  1. ChangedInsertExitModeAction.typetoOTHER_SELF_SYNCHRONIZED
  2. Added manual write lock handling using try-catch around editor.exitInsertMode(context)
  3. Implemented the 99% normal case / 1% fallback approach as you recommended

The ESC key now works properly in read-only files without hanging, while preserving all existing functionality for writable files. I've also added tests to cover the read-only file scenarios.
All existing tests continue to pass. Would you like me to make any adjustments to this approach?

Sorry, I realized there are still some parts I need to refine further — I’ll update the patch soon.

@NaMinhyeok
Copy link
Contributor Author

Follow-up Update

While refining the patch further, I realized there was another important case that needed attention.
I’ve adjusted the implementation to ensure consistency and added more safety checks.

To make sure ESC behaves correctly in non-writable files, I cherry-picked the related commit and tested it separately.
From my testing, it appears to be working as intended, but please let me know if you think I should add more coverage.

Key Issue Resolved: 3ia<Esc> Repeat Functionality

The main problem was that 3ia<Esc> wasn't generating aaa as expected in read-only files.

Solution

  • Restructured initInsert() to handle repeat cases (isDotRepeatInProgress) separately from new insert cases
  • Moved read-only checks to appropriate timing:
    • Repeat cases → before repeat execution
    • New insert cases → before mode entry
  • This allows 3ia<Esc> to properly store the repeat count during 3i, then execute the repeat during <Esc>

Additional Note

After checking the actual Vim behavior, I found that in the case of 3ia, Vim enters Insert mode immediately when the i is pressed.
Following this, I also made sure that our implementation switches to Insert mode at the moment i is input.


Additional Fix

During testing, I also noticed that the Command.Type change caused property-based tests to crash with "Unexpected action type" errors.

Resolution

  • Extended the CommandBuilder and Argument systems to safely handle all EditorActionHandlerBase types

Writbale

2025-09-01.11.56.38.mov

Writable other action(like 3ia)

2025-09-01.11.57.51.mov

non-writable

2025-09-01.11.57.31.mov

@AlexPl292 @wxh06
Do these changes align with what you were expecting? I’d be happy to adjust further if needed.

@AlexPl292
Copy link
Member

Thank you for the updates and the detailed description! However, I would like to point out some concerns regarding the current changes:

  • In the second commit, we wrap the command with try-catch and intercept any kind of exception. I don't think this is the proper approach – we may catch exceptions that are not related to our issue. Instead, it would be better to identify the places where the write lock is actually needed and add writability checks there. Please try to review the execution workflow for a command like 3ia<esc>.
  • Could you please clarify the changes in the last commit? It seems to me like the statement was just moved within the if-else branches, making it essentially the same as before, doesn't it? Also, the new construction doesn't appear to be used.

Thank you!

@AlexPl292
Copy link
Member

@claude /review

Copy link

claude bot commented Sep 5, 2025

Claude encountered an error —— View job

Failed with exit code 128

I'll analyze this and get back to you.

@citizenmatt
Copy link
Member

I'm not sure about the change to CommandBuilder. We don't want to support arbitrary actions as arguments.

The way CommandBuilder works without this change is that an action argument must be a motion of some kind - MotionActionHandler like w or b, TextObjectActionHandler like iw or ia or ExternalActionHandler which is a way to wrap IntelliJ actions as motions, like AceJump.

More specifically, the argument is what comes after the operator, so dw is a command with DeleteMotionAction and an argument of MotionWordRightAction. The argument has to be a motion or text object. None of the operator actions are expecting anything other than a motion, so I'm not sure what this is intended to do. And I can't see where this new constructor is used.

Can you explain a bit more what this change is about, please?

@citizenmatt
Copy link
Member

BTW, a good file to test this with would be ~/.ideavimrc. Since it's outside of the project, it's initially considered not writable, and the IDE will show a dialog when you try to modify the file. We can check if the dialog shows when entering Insert mode.

@NaMinhyeok
Copy link
Contributor Author

Thank you both @AlexPl292 and @citizenmatt for the detailed feedback. I realize that both of my approaches were fundamentally flawed, and I appreciate you taking the time to explain the proper architecture and concerns.

You're absolutely right that:

  1. The try-catch approach is too broad and may catch unrelated exceptions
  2. The CommandBuilder changes were misguided and don't align with the command argument system
  3. The second commit was essentially moving code around without meaningful improvement

This is clearly not as easy as I initially thought, and I need to take a step back and understand the proper workflow and architecture better.

I'll take the time to properly:

  1. Study the command execution workflow - especially for commands like 3ia<esc>
  2. Understand where write operations actually occur in the codebase
  3. Remove all the problematic changes (CommandBuilder modifications, broad try-catch)
  4. Test with ~/.ideavimrc as suggested to understand the real-world behavior
  5. Implement precise writability checks only where actually needed

The core goal remains ensuring ESC always works in read-only files, but I need to do this properly within the existing architecture rather than working around it.

I'll come back with a cleaner, more targeted approach once I've done the proper analysis.

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.

4 participants