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 delegated properties #616

Merged

Conversation

citizenmatt
Copy link
Member

This PR introduces strongly typed delegated properties to get and set option values. Options of primitive type are implemented as read/write properties with native types, e.g. String, Int and Boolean, which makes it very easy to get/set an option value from code. String list options are represented as a read only property of type StringListOptionValue.

E.g.:

val ignorecase: Boolean = injector.globalOptions().ignorecase
injector.options(editor).selection = "exclusive"
val scrollOffset: Int = injector.options(editor).scrolloff
val clipboard: StringListOptionValue = injector.globalOptions().clipboard

The options are accessed via injector.globalOptions() and injector.options(editor) functions, similar to how OptionsValueAccessor was used. The OptionsValueAccessor class has been removed in favour of the strongly typed properties. The VimOptionGroup.setOptionValue function can be used to set an option found at runtime via VimOptionGroup.getOption.

The StringListOptionValue type implements List<String>, so the values can be iterated, or a specific item can be checked with in or contains. It also implements the appendValue, prependValue and removeValue helpers to make it easy to modify the string list. The underlying string is available through the value property.

String list options can be used like:

val hasIdeaput: Boolean = injector.globalOptions().clipboard.contains("ideaput")
injector.globalOptions().clipboard.prependValue("ideaput")
if ("ideaput" in injector.globalOptions().clipboard) { ... }

// Java
for (String s : injector.globalOptions().getMatchpairs()) { ... }

While it is conceptually very similar to StringOption, the implementation of list vs not-list operations are very different, and having a separate type will allow us to do more interesting things with overloading when we introduce delegate properties
Also migrates tests to use new properties
Also fixes some incorrect usages of local options as global, e.g. 'ideajoin' and 'scroll'. There are some options that should be local that are only ever accessed at global scope. These need fixing in the future, e.g. 'iskeyword', 'matchpairs' and 'virtualedit'
* Convenience function to get the IntelliJ implementation specific option accessor for the given editor's scope
*/
@Suppress("UnusedReceiverParameter")
public fun VimInjector.ijOptions(editor: VimEditor): EffectiveIjOptions = getEffectiveIjOptions(editor)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not too keen on this, but we can't upcast VimOptionGroup to IntelliJ's OptionGroup because we re-implement the VimOptionGroup in tests. An alternative would be to create an IjVimOptionGroup interface to access IntellIJ specific options, and have the test class also implement this interface. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean that we reimplement it by OptionsTracer? If yes, I'd comment out OptionsTracer as this should not affect the architecture decisions.

@AlexPl292 AlexPl292 requested review from AlexPl292 and lippfi and removed request for AlexPl292 and lippfi April 4, 2023 17:30
@AlexPl292
Copy link
Member

This looks good to me! But still, I'd love to see some historical notes like:

  • We used to have "access by name" approach, but we consider this as a better solution (access by field). Please don't switch back to "access by name".
  • We used to have "string as list" approach, but we consider this as a better solution (StringListOption class). Please don't switch back to "string as list".

Also, I'd love to see some small instructions on how to add a new option. Like what classes should be affected by that.

Copy link
Contributor

@lippfi lippfi left a comment

Choose a reason for hiding this comment

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

Everything is so clean now, thank you!

@citizenmatt
Copy link
Member Author

@AlexPl292 Just pushed some minor updates to address comments

@AlexPl292 AlexPl292 merged commit 29bd7cb into JetBrains:master Apr 26, 2023
@citizenmatt citizenmatt deleted the refactor/option-delegated-properties branch April 26, 2023 08:19
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