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

Settings: Improve completions of known values #258

Merged
merged 3 commits into from
Sep 19, 2019

Conversation

deathaxe
Copy link
Member

Had a look at 8b904c6 and found some possible edge cases to fix.

While looking into it, I found the (default) marker not to be added to color_scheme and theme completions. Fixed it by extending the format_completion_item().

Finally found the color_scheme completions missing the sublime-color-scheme files and fixed it.

Note:

The extended format_completion_item() could be used in _completions_from_comment() and _completions_from_default() to add the (default) mark, which would result in _marked_default_completions() not being needed anymore. Left that for now as I am not sure whether it was the better alternative.

The main reason for extending format_completion_item() was to avoid breaking the description part of the completion label by calling _marked_default_completions() on color_scheme, encoding or theme completions.

DeathAxe added 3 commits September 19, 2019 19:43
This commit modifies 8b904c6 by
handling encoding completions exactly the same way as `color_scheme`
and `theme` keys.

Therefore the decision to return completions for `default_encoding` and
`fallback_encoding` is made directly in the `_value_completions_for`
method, which is meant to do exactly this.

The if-branches are sorted by the key name rather than logical meaning.
Same for the definition of the called methods.

There is no need to merge comments and default values into the result
if a complete list of values is already available.

The `_known_completions()` method automatically marks the default encoding value.
This commit applies the `(default) ` mark to the default `color_scheme` and `theme` values.
Up to this commit color schemes with the "new" sublime-color-scheme
format were not added to the completions for the `color_scheme` settings.
Copy link
Member

@FichteFoll FichteFoll left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes. I'll apply some formatting changes as I don't personally agree with this style of line splitting in some cases.

completions.add(format_completion_item(
value=scheme_path, is_default=scheme_path == default,
label=file_name, description=package))
label=name, description=package))
Copy link
Member

@FichteFoll FichteFoll Sep 19, 2019

Choose a reason for hiding this comment

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

Now that I see this, tmTheme files need to be specified by path, not by name. I'll fix this. Nvm, the value is still correct.

@FichteFoll FichteFoll merged commit 6abee18 into SublimeText:master Sep 19, 2019
@FichteFoll FichteFoll added this to the 3.2.11 milestone Sep 20, 2019
@deathaxe deathaxe deleted the pr/settings branch September 20, 2019 15:07
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