Skip to content
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

Fleet #824

Merged
merged 46 commits into from
Feb 23, 2024
Merged

Fleet #824

merged 46 commits into from
Feb 23, 2024

Conversation

lippfi
Copy link
Contributor

@lippfi lippfi commented Feb 16, 2024

No description provided.

import javax.swing.KeyStroke

public class CommandBuilder(private var currentCommandPartNode: CommandPartNode<LazyVimCommand>) {
private val commandParts = ArrayDeque<Command>()
public class CommandBuilder(private var currentCommandPartNode: CommandPartNode<LazyVimCommand>): Cloneable {
Copy link
Member

Choose a reason for hiding this comment

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

synchronized(lock) {
val keyHandler = KeyHandler.getInstance()
if (keyHandler.keyHandlerState != originalState) {
logger.warn("Unexpected editor state. Aborting command execution.")
Copy link
Member

Choose a reason for hiding this comment

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

Do we miss return here?

@AlexPl292
Copy link
Member

AlexPl292 commented Feb 16, 2024

Review notes:

  • Review each commit and add a description about WHY this change was made
  • clearMessage from tests can be moved to the tearDown of the VimTestCase
  • Refactor and split this huge commit: 8514fe1. Here are some hints what can be extracted:
    • Making handleKey argument not-nullable
    • Wrapping handleKey with a synchronized
    • Step-by-step extraction of the key processors instead of extracting all processors in one commit
  • Commits
    Attempt to add annotation-processors as an artifact to vim-engine and Avoid using annotation-processors in vim-engine should be squashed or even removed from this branch. Is this related to this refactoring?
  • Clean up merge branch master and Fix merge commits.

Additional notes:

  • Global state can be removed at all. This was an experiment, there is no need to handle it separatelly
  • VimStateMachine.getInstance() can be inlined and removed. Any argument can be replaced with VimEditor.

@lippfi lippfi force-pushed the fleet branch 2 times, most recently from 9855865 to 0d7cfc3 Compare February 18, 2024 01:11
lippfi and others added 25 commits February 23, 2024 15:09
Rationale:
1. A much more experienced developer, whom I highly respect, suggested to empty VimStateMachineImpl constructor in his TODO comment.
2. I aim for VimStateMachine to be a data class rather than being a container for both data and complex logic.
3. From an architectural perspective, it is more correct. Editors do have state (or they may possess a single global state if the corresponding option is set), but a state does not own an editor.
1. Listeners now disposed after turning plugin off
2. Change widget listeners to be recreated on plugin toggle
3. Add CaretVisualAttributesListener
It will not only simplify VimStateMachine, but also help us to support multi-editor macros in future
…with the new UI and will be replaced with widget
It unnecessarily binds mappingState to mode and thus to editor. And we want to simplify things and have a single MappingState instead of multiple of them
We do not need multiple commandBuilder, digraphSequence or mappingState and this class will be a singleton containing them
Applying default values may lead to unexpected results, especially if we sometimes want to use the global state (IJ), and at other times, its clone for asynchronous processing (Fleet).
It will help us to build the KeyProcessResult that we need for asynchronous key processing
It will help us to have a more modular KeyHandler in future (chain of different consumers)
It shouldn't be retested on partial reset
# Conflicts:
#	vim-engine/src/main/kotlin/com/maddyhome/idea/vim/api/VimEditorGroup.kt
#	vim-engine/src/main/kotlin/com/maddyhome/idea/vim/impl/state/VimStateMachineImpl.kt
@lippfi lippfi merged commit 00808af into master Feb 23, 2024
2 checks passed
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.

2 participants