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 option usage and introduce accessor API #589

Merged
merged 23 commits into from
Jan 18, 2023

Conversation

citizenmatt
Copy link
Member

First things first - sorry about the size of this PR. It's a bit of a monster, but I don't think I can make it any smaller 😊. It would be possible to mostly split the PR into production code changes and test code changes, but there are some renames that affect all files, so this wouldn't reduce the number of files to review, only the number of commits. Fortunately, the changes are mostly mechanical refactorings, with no significant changes to business logic.

This PR is primarily about refactoring and minimising usages of OptionService, so that a subsequent PR can refactor that service with less impact, which will make that PR easier to review. These subsequent changes are to support :set wrap - an option that needs to be backed by IDE APIs rather than a dictionary value, as there doesn't appear to be an event listener for editor settings to sync the dictionary value.

Specifically, this PR will:

  • Introduce a friendly API to consume options values in code, e.g. getIntValue to work with 'scrolloff'. This OptionValueAccessor is accessed via injector.options(editor) or injector.globalOptions(), and decouples the majority of production code from OptionService. It also simplifies consuming options. Code that needs to administer options, such as in response to :set something+=whatever will still use OptionService.
  • Introduce the same accessor into tests via the helper functions VimTestCase.options() and VimTestCase.optionsNoEditor() (for accessing global options). This again simplifies the calling code, and reduces coupling to OptionService.
  • Replace test usage of OptionService.setOptionValue, setOption, unsetOption, etc. by using the :set command. Tests now set options in the same way that users do - :set scrolloff=10, set hlsearch ignorecase, etc.
  • Some other small refactorings

@AlexPl292
Copy link
Member

Well, this looks for me :)

Also, I'd like @lippfi to take a look as he worked on this OptionService a lot

@AlexPl292 AlexPl292 requested review from lippfi and AlexPl292 January 13, 2023 08:52
@AlexPl292
Copy link
Member

AlexPl292 commented Jan 13, 2023

Also, just a fancy idea: what if we'll change our current option listeners to coroutine API with flows and stuff? Not sure if this will bring any actual value, but this may be interesting plus, it's a movement into the common direction "use kt coroutines".

@citizenmatt
Copy link
Member Author

I think we should definitely look at coroutines for other listeners, but I don't really think there's a need for option change listeners. If anything, I'd be happy to get rid of them! They look like an API that can be used to get notification that options have changed, but they're really an implementation detail to refresh the UI when the option changes. I'd be tempted to move them to the Option class itself.

they're a good fit for option change listeners. If anything, I'd like to get rid of them if possible! They look like an API, but they're really an implementation detail, for refreshing behaviour in response to :set

@citizenmatt citizenmatt force-pushed the feature/options-accessor-api branch from 6f85734 to a91706e Compare January 18, 2023 15:04
@AlexPl292 AlexPl292 merged commit 308e8bf into JetBrains:master Jan 18, 2023
@citizenmatt citizenmatt deleted the feature/options-accessor-api branch January 18, 2023 21:32
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