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

[PackageDev] Add support for var() #186

Open
MattDMo opened this issue Jun 12, 2021 · 15 comments
Open

[PackageDev] Add support for var() #186

MattDMo opened this issue Jun 12, 2021 · 15 comments
Labels
S: triage Issue needs triage.

Comments

@MattDMo
Copy link

MattDMo commented Jun 12, 2021

Description

The support for PackageDev themes and color schemes is great - it's what I most often use this plugin for. Would it be possible to add previews for the simple "var(some_color_variable)" syntax?

For example, in a color scheme, I define a variable and then use it for a global:

{
    "variables": {
        "blue": "#0042FF"
    },
    "globals": {
        "selection": "var(blue)"
    }
}

The color preview shows up next to the #0042FF as it should, but I would like that same preview to show up next to var(blue). I don't use any of the color() mod function options, but I imagine those that do would probably like to see previews of those, too (if possible, given they're implemented within Sublime and not exposed via the API, that I know of). However, even if you can't do that or it would take a long time, I should think it would be fairly straightforward to highlight var(). Unless the variables themselves are made using color()...

@gir-bot gir-bot added the S: triage Issue needs triage. label Jun 12, 2021
@facelessuser
Copy link
Owner

Here's the thing. I have the capability to parse, that part isn't hard. But this requires us to parse the whole file every time to get the context of all the var() values.

Currently, ColorHelper works on a sliding window. Everything that is visible (both horizontally and vertically) gets parsed and has previews added. Everything outside that window does not.

This is mainly done for performance and is done in a general way. Additionally, any file that does something like this must have language context. In PackageDev, we must parse the file as JSON grab anything in variables and store them. In something like SCSS, we have to look for things like $var: color. I mention this because if I add support for one language, others will be asking for it in others.

In such cases, you almost need something like a language server to provide such info asynchronously.

The idea was to keep things generic but simple. Or as simple as can be expected considering the complexity of specifying different color types per file along with other syntax-specific preferences. I'll be the first to admit the config file is fairly complex currently.

Adding in variable detection definitely elevates the level of complexity, and then we must solve performance-related issues as we would then be scanning the entire file each time. A sliding window is no longer going to cut it. And the approach most assuredly has to be flexible enough to some mechanism to provide language-specific parsing.

For these reasons, such a feature has not been added as of yet.

@MattDMo
Copy link
Author

MattDMo commented Jun 13, 2021

I see, I had forgotten about the sliding window part of it. Would pairing up with LSP-json and LSP-css help? I have no idea, just throwing things out there...

@facelessuser
Copy link
Owner

@rwols I believe had made a suggestion while I was kneedeep in re-writing ColorHelper about somehow connecting LSP with ColorHelper. At the time I was so overwhelmed with work that I never had a chance to fully look into it.

Theoretically, we have the capability to apply the variables if we know them: https://github.com/facelessuser/ColorHelper/blob/master/custom/st_colormod.py#L528.

I'm not sure at this time what is possible with LSP and what is not. For instance, I'm not sure if LSP completely resolves colors in CSS or not, or just resolves variables. Can I get that on-demand or is the only way for LSP to send that info when it is ready, and you just have to be prepared to accept it?

So, is it possible? Maybe. We'd have to figure out how to get these two separate plugin systems talking. If we just have to be ready for LSP at any time, maybe there could be a way that ColorHelper just accepts the context data we need for a given view, and then when we are evaluating patterns in a view, we then check if we have context data.

They fundamentally approach things differently, but if we could work out some kind of way to pass context, maybe it would be possible. But, the important thing is that whatever we do on the ColorHelper side has to be generalized as we don't want language-specific code in the core, and if we do need language specific code, it has to be done in such a way that we provide a hook and point it at some external code that we can run on the fly.

@AmjadHD
Copy link

AmjadHD commented Jan 20, 2022

Can you make it work for built-in variables like --background and --accent etc. ?

@facelessuser
Copy link
Owner

Can you make it work for built-in variables like --background and --accent etc. ?

The problems for built-in variables and other custom variables remain the same. Nothing is made easier or faster by only adding support for built-in variables.

@AmjadHD
Copy link

AmjadHD commented Jan 21, 2022

Oh, I meant in the context of the command palette.

@facelessuser
Copy link
Owner

Can you elaborate? I'm not sure what your mean.

@AmjadHD
Copy link

AmjadHD commented Jan 28, 2022

image

@facelessuser
Copy link
Owner

Are you proposing it always loads the variables from the currently loaded color-scheme? Looks at the current active buffer and loads variables from there? Some combination?

While I'm not sure whether we'd go this route or not, I'm not quite sure I understand the expectation with this proposal.

@AmjadHD
Copy link

AmjadHD commented Jan 29, 2022

var(--name) comes from the global CS, and var(view.--name) from the view.

@facelessuser
Copy link
Owner

I don't think var(view.--name) is a thing.

@AmjadHD
Copy link

AmjadHD commented Jan 30, 2022

Why not ?

@facelessuser
Copy link
Owner

One of the reasons I'm not too keen on including variables is because it isn't even as simple as just get the current variables.

There are complexities when trying to perform such variable resolution with Sublime themes. You have to take in consideration file overloads and such which would require us to scan all color schemes every time. We'd have to find all the like-named color schemes, overlay them in the proper order, then find all the variables, and then resolve them all. ScopeHunter used to do this to get proper color scheme variables when it was still manually parsing color schemes.

Why not ?

Because var(view.--name) doesn't exist as a convention. It breaks the CSS variable identifier rule with the dot. This makes it a little more complicated to provided patterns for it. I assume you are proposing this as a new thing. I assume the thinking is that the current global scheme is considered unless prefixing view. and then the view's color scheme (which may not match the global one) is then considered? Or do you mean the scheme loaded in the view's buffer (the text)? If the latter, I assume we'd have to check the path against Sublime's package path to see if it is within the said path and if so, try to resolve it with overloads? Or maybe just ignore overloads?

Anyways, you can see how this gets more and more complicated.

Now, we could probably simplify all this by simplifying this process by only evaluating the text in the given active view and not scanning all the overloads.

Anyways, I get the gist of what you are proposing. I'm not sure how I feel about going down this road right now, but I'll keep it in mind as a possibility.

@AmjadHD
Copy link

AmjadHD commented Jan 31, 2022

I don't think considering overloads is necessary, just go the easy way:
var(view.--name) is got from view.style() and var(--name) from sublime.ui_info(), all values resolved.
As for view. prefix, you can make it opt-in and/or only available in the the command palette.

@facelessuser
Copy link
Owner

I don't think considering overloads is necessary, just go the easy way:
var(view.--name) is got from view.style() and var(--name) from sublime.ui_info(), all values resolved.
As for view. prefix, you can make it opt-in and/or only available in the the command palette.

If all you want is background or accent, sure, but it really isn't exposing variables as much as it is exposing some color scheme settings. I'm not sure it's worth it for such limited variable access. TBH, if I need to mix background with something, I just use ScopeHunter, copy the background, and plug it into Sublime ColorMod mixer.

I'm also not really a fan of view.--name if I'm being honest as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: triage Issue needs triage.
Projects
None yet
Development

No branches or pull requests

4 participants