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

Use a dropdown setting to switch between the ways of visualizing inactive regions (background color vs. opacity) #643

Open
postcoital-solitaire opened this issue Jun 23, 2024 · 10 comments
Labels
enhancement New feature or request

Comments

@postcoital-solitaire
Copy link

Initially started as this question of mine, but I think it belongs as a feature request. Similar thing has been already implemented, but it seems a bit crude in my opinion. I think it should be handled like a drop-down menu with options rather than multiple different settings pointing to each other like used only if xxx. Options may include:

  • disable - no change at all (like current opacity=1, useBackground=false);
  • opacity - just opacity (opacity=0.55, useBackground=false);
  • workbenchColors - uses currently present clangd.inactiveRegions.background and maybe create another one for foreground? An example would be this picture from the question about how Qt Creator does that:
    img
  • workbenchColorsOpacity - like workbenchColors but also utilizes opacity setting;
  • cppBuiltInColors - use already existing options from built-in cpp extension (like this answer shows, but sadly doesn't work for clangd).

Not saying all of these should exists, but I would be glad to see some setting to set the overall foreground of inactive region, either in workbench color customizations or as a separate setting. I really like how it looked in QTC. Shouldn't be too hard to implement, right? We already have working drop-down menus and almost all options already exist.

@postcoital-solitaire postcoital-solitaire added the enhancement New feature or request label Jun 23, 2024
@HighCommander4
Copy link
Contributor

HighCommander4 commented Jun 23, 2024

Similar thing has been already implemented, but it seems a bit crude in my opinion. I think it should be handled like a drop-down menu with options rather than multiple different settings pointing to each other like used only if xxx.

I definitely agree a drop-down like the one you describe would be nicer! I just wasn't sure whether / how it can be done using VSCode's APIs / extension points.

If this is possible to do, PRs to change it are very welcome, and I'm happy to review them.

and maybe create another one for foreground?

A note on foreground color: one of the specific goals in #193 was to not set a foreground color on the entirety of an inactive region, so that the non-foreground style (background or opacity) that is set can be combined with foreground colors set by VSCode's client-side (TextMate) highlighting engine.

This way, e.g. keywords in inactive regions still remain bold, strings are still colored differently than identifiers, etc. This makes the inactive code easier to read and edit than if it was all one color, while still making it clear that it's inactive.

I suppose we could add an option to make it all one color if you think that would be useful, but we'd definitely want to keep an option to combine background or opacity styles with TextMate foreground colors.

  • cppBuiltInColors - use already existing options from built-in cpp extension (like this answer shows, but sadly doesn't work for clangd).

I'm not following this one, could you elaborate please?

@postcoital-solitaire
Copy link
Author

I suppose we could add an option to make it all one color if you think that would be useful, but we'd definitely want to keep an option to combine background or opacity styles with TextMate foreground colors.

I understand why keeping the foreground colors was desired, and I guess most people would want that. But just for the weirdos like me, a one color foreground would be nice. The styles (bold, italics etc.) would likely leak in (like in QTC screenshot), but it's not big of a deal, in my opinion. Of course, other options, like opacity and background, should still be available to others that prefer them.

If this is possible to do, PRs to change it are very welcome, and I'm happy to review them.

I'd happily look into that, but I'm no node.js and/or vs-code extension expert or even a beginner. I tried creating an extension some time ago, not related to this issue, and I just dropped it because of difficulties of learning a new language and pipeline. Not to mention expanding on an already existing big extension someone had already made. That seems dreadful for me.

I'm not following this one, could you elaborate please?

VSC has a built-in C/C++ extension, which has inactive region tweaking options:

cpp

They seem to do the same thing clangd does with its opacity and background options for inactive code. But it only works if IntelliSense is enabled, which is not, if we're talking clangd. What I'm suggesting is to add an option which, instead of using clangd's own settings for background and opacity, would rely on already existing settings from this built-in extension. No additional settings needed, just use these values. I'm not sure if extensions can access other extensions' settings, but if they can, this seems pretty easy to implement.

@HighCommander4
Copy link
Contributor

HighCommander4 commented Jun 25, 2024

If this is possible to do, PRs to change it are very welcome, and I'm happy to review them.

I'd happily look into that, but I'm no node.js and/or vs-code extension expert or even a beginner. I tried creating an extension some time ago, not related to this issue, and I just dropped it because of difficulties of learning a new language and pipeline. Not to mention expanding on an already existing big extension someone had already made. That seems dreadful for me.

Understood.

I think a possible conclusion here is "a dropdown where different options have different associated data types (e.g. one option requires selecting a color, another option requires setting an opacity value)" is not a type of setting that the VSCode extension API supports.

I'm not following this one, could you elaborate please?

VSC has a built-in C/C++ extension, which has inactive region tweaking options:

cpp

They seem to do the same thing clangd does with its opacity and background options for inactive code. But it only works if IntelliSense is enabled, which is not, if we're talking clangd. What I'm suggesting is to add an option which, instead of using clangd's own settings for background and opacity, would rely on already existing settings from this built-in extension. No additional settings needed, just use these values. I'm not sure if extensions can access other extensions' settings, but if they can, this seems pretty easy to implement.

Thanks, I understand now.

While I haven't been involved in vscode-clangd from the beginning, my understanding is that the project has made a deliberate choice not to try to use the same settings as vscode-cpptools. For example, we do not pay any attention to c_cpp_properties.json. I think it's unlikely we would deviate from that for this particular feature.

@postcoital-solitaire
Copy link
Author

While I haven't been involved in vscode-clangd from the beginning, my understanding is that the project has made a deliberate choice not to try to use the same settings as vscode-cpptools.

Ah, I see. Then, this option should not exist. But still, these settings might be a good example of how to organize inactive region highlighting. These four settings are enough to achieve everything I have proposed, without the use of the drop-down menu.

Btw, why was this choice made? I don't see any particular reason not to base a C/C++ extension off of a built-in C/C++ extension. Or am I reading to much into this and this only affects settings?

I think a possible conclusion here is "a dropdown where different options have different associated data types (e.g. one option requires selecting a color, another option requires setting an opacity value)" is not a type of setting that the VSCode extension API supports.

I don't understand, how are different types a part of this?

@HighCommander4
Copy link
Contributor

HighCommander4 commented Jun 27, 2024

While I haven't been involved in vscode-clangd from the beginning, my understanding is that the project has made a deliberate choice not to try to use the same settings as vscode-cpptools.

[...]

Btw, why was this choice made? I don't see any particular reason not to base a C/C++ extension off of a built-in C/C++ extension. Or am I reading to much into this and this only affects settings?

Just to clarify, vscode-cpptools may be the extension for C/C++ code that VSCode recommends by default (presumably because both VSCode itself and vscode-cpptools are authored by Microsoft), but it's not "built-in" in the sense that it's bundled with a base VSCode installation, or that its presence can be assumed by other extensions.

vscode-cpptools and clangd are basically alternatives to each other. Both provide a language server and associated editing features. vscode-cpptools is proprietary (at least the language server is), and clangd provides an open-source alternative.

As such, I don't think there's any meaningful way in which clangd could be "based on" vscode-cpptools. At most, it could try to respect vscode-cpptools' settings. The decision not to do so pre-dates my involvement in the project, but I can speculate as to the reasons:

  • For specifying compile flags (what vscode-cpptools does using c_cpp_properties.json), the clang ecosystem already had a mature solution called compilation databases, which was widely adopted by build systems and other clang-based tools like clang-tidy, so it made sense to reuse that.
  • There are other things for which clangd needs its own settings anyways since vscode-cpptools doesn't have an equivalent.
  • So if we did try to respect some vscode-cpptools settings, there would be a mixture of settings specific to vscode-cpptools, settings specific to clangd, and settings respected by both. That seems more confusing than just keeping the two extensions' settings separate altogether.

I think a possible conclusion here is "a dropdown where different options have different associated data types (e.g. one option requires selecting a color, another option requires setting an opacity value)" is not a type of setting that the VSCode extension API supports.

I don't understand, how are different types a part of this?

Maybe I misunderstood the suggestion; I thought you meant having a dropdown where e.g. some (but not all) options include a color picker contained within them.

[The vscode-cpptools settings] might be a good example of how to organize inactive region highlighting. These four settings are enough to achieve everything I have proposed, without the use of the drop-down menu.

Well, those look fairly similar to ours, down to the notes like "This setting only applies when inactive region dimming is enabled."

The main difference is that they also have an option for a foreground color, so it seems like that's the main ask here.

@postcoital-solitaire
Copy link
Author

Just to clarify, vscode-cpptools may be the extension for C/C++ code that VSCode recommends by default (presumably because both VSCode itself and vscode-cpptools are authored by Microsoft), but it's not "built-in" in the sense that it's bundled with a base VSCode installation, or that its presence can be assumed by other extensions.

I understand now. I must've confused the ms-vscode.cpptools for the vscode.cpp, which is actually built-in and called C/C++ Language Basics. I somehow assumed that the settings I was talking about are from the Basics one, while they are actually not, the Basics one has no settings whatsoever. Sorry for that. Now it actually makes perfect sense why the decision in question was made, and I actually respect it.

vscode-cpptools and clangd are basically alternatives to each other. Both provide a language server and associated editing features. vscode-cpptools is proprietary (at least the language server is), and clangd provides an open-source alternative.

Not really relevant to the subject of the issue (as pretty much this whole tangent regarding the cpp extension), but I thought ms-vscode.cpptools was pretty much mandatory for any C/C++ development on VSC. And now I actually see that clangd replaces it, the only thing clangd doesn't seem to support is debugging. The more you know, I guess.

Maybe I misunderstood the suggestion; I thought you meant having a dropdown where e.g. some (but not all) options include a color picker contained within them.

What I meant is a menu like this one from Error Lens which let's user pick what to display in editor gutter, and additional options to fine tune how to display it:

errorlens

To my mind, something similar to this would be more user-friendly. Instead of deducing what different options do, it's easier to pick an 'operation mode' of a certain feature, a behaviour, if that makes sense. And then user can fine tune it with some settings, like color, opacity etc. This does not involve any trickery with different types and seems fairly simple to implement. This was my main point in the original comment (and of course foreground option).

Well, those look fairly similar to ours.

True. I regarded this style as a 'back-up plan', if the drop-down menu idea was rejected. One problem, however, is that clangd's clangd.inactiveRegions.background is not explicitly shown in settings. It's a workbench color customization, which again is not so user-friendly. I myself have discovered it purely by accident. The ones from Microsoft's extension are explicitly in the settings and can be customized to user's liking.

@HighCommander4
Copy link
Contributor

HighCommander4 commented Jul 3, 2024

Maybe I misunderstood the suggestion; I thought you meant having a dropdown where e.g. some (but not all) options include a color picker contained within them.

What I meant is a menu like this one from Error Lens which let's user pick what to display in editor gutter, and additional options to fine tune how to display it.

Ok, thanks for clarifying! If we're just talking about replacing the checkbox ("use background highlight rather than opacity") with a dropdown with two options ("background highlight" and "opacity"), that should be straightforward to change.

One problem, however, is that clangd's clangd.inactiveRegions.background is not explicitly shown in settings. It's a workbench color customization, which again is not so user-friendly. I myself have discovered it purely by accident. The ones from Microsoft's extension are explicitly in the settings and can be customized to user's liking.

I think the idea behind making it a workbench color customization is that themes can specify a value for it, so that when you change themes, the background color can potentially also change in a way that's harmonious with the other colors in the theme.

Do you know how Microsoft's extension deals with this?

@postcoital-solitaire
Copy link
Author

Ok, thanks for clarifying! If we're just talking about replacing the checkbox ("use background highlight rather than opacity") with a dropdown with two options ("background highlight" and "opacity"), that should be straightforward to change.

Adding a foreground highlight should also be easy, although I'm not sure if it's anything similar to adding a background highlight.

I think the idea behind making it a workbench color customization is that themes can specify a value for it, so that when you change themes, the background color can potentially also change in a way that's harmonious with the other colors in the theme.

I was thinking about leaving it a color customization but also adding a separate setting, which would override the color customization. For example, when no value is specified, use theme color, otherwise use the value specified. Maybe leave just the color customization without a separate setting, but specify in the hint to the drop-down menu option, like backgroundHighlight: 'Highlight inactive region's background. Use clangd.inactiveRegions.background workbench customization to specify color.'

Do you know how Microsoft's extension deals with this?

Doesn't seem that Microsoft's extension creates a workbench color customization for this. I can't really think of a reason why.

@HighCommander4
Copy link
Contributor

Thanks. To summarize, the proposal is for two changes:

  1. Change the kind of the setting used to switch between the "background color" style and the "opacity" style, from a checkbox to a drop-down.
  2. Add a third option to the drop-down to apply a foreground color to inactive regions.

Both of these sound reasonable to me. I'll update the title of this issue to track the first one, and file a follow-up issue to track the second one.

@HighCommander4 HighCommander4 changed the title Further improving inactive regions / disabled code customizations. Use a dropdown setting to switch between the ways of visualizing inactive regions (background color vs. opacity) Jul 12, 2024
@HighCommander4
Copy link
Contributor

I'll update the title of this issue to track the first one, and file a follow-up issue to track the second one.

#654 is the second issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants