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

Refactor VimOptionGroupBase #788

Merged
merged 14 commits into from
Jan 4, 2024

Conversation

citizenmatt
Copy link
Member

The VimOptionGroupBase class has become a bit of a god class/kitchen sink class. It has too many responsibilities, and is confusing to reason about. It handles setting/getting option values, initialising options for new windows and buffers, notifying listeners of changes and maintaining a cache of parsed effective values (e.g. parsing the contents of 'guicursor').

This PR splits the responsibilities into separate classes that VimOptionGroupBase uses. The classes are all implementation details, so remain in the same VimOptionGroupBase.kt file, but at least encapsulate responsibilities.

The majority of the changes are renames, extracting classes, and other similar refactoring. There is very little behaviour change, although a couple of minor bugs have been fixed.

This refactoring is the basis for the next PR, which will modify the option storage to allow custom options that are powered by IntelliJ settings (e.g. 'wrap', based on IntelliJ's soft wrap settings).

@citizenmatt citizenmatt force-pushed the feature/options-refactor branch from 627d1d5 to 3ba568b Compare January 3, 2024 10:57
@citizenmatt citizenmatt force-pushed the feature/options-refactor branch from 3ba568b to d9f7e5c Compare January 4, 2024 10:19
@AlexPl292
Copy link
Member

Thank you for the refactoring!

This refactoring is the basis for the next PR, which will modify the option storage to allow custom options that are powered by IntelliJ settings (e.g. 'wrap', based on IntelliJ's soft wrap settings).

Can I hear about your ideas on how this should work?

Also, I see a lot of changes around listeners. JFI, I think at some moment they should be moved from the old observer pattern to the Kotlin flows.

@AlexPl292 AlexPl292 merged commit 825b62a into JetBrains:master Jan 4, 2024
4 checks passed
@citizenmatt citizenmatt deleted the feature/options-refactor branch January 4, 2024 14:03
@citizenmatt
Copy link
Member Author

The changes around listeners here are only about separating responsibilities. I have no other plans for them right now, so don't see any issues with using flow in the future.

I'll have a PR ready very soon for 'wrap' and hopefully a few other options. I started to describe it here, but it was becoming a description of the PR, so it's probably going to be easier just to pull together a draft PR as soon as possible - hopefully by tomorrow. I'm happy with the approach - this was always the end goal of all the options changes recently, and I think it's ended up both easy and intuitive 🤞 Will share ASAP.

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