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

Themes: add option for image background color #964

Merged
merged 4 commits into from Sep 17, 2021

Conversation

NickCraver
Copy link
Contributor

Prerequisites

  • I have read and understood the CONTRIBUTING guide
  • The commit message follows the conventional commits guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added / updated (for bug fixes/features)
    • I think the above 2 are true since this appears to be generated as part of scripts. To the best of my ability/understanding I have tweaked the needed bits but apologies if I missed something and I'm happy to remedy.

Description

I wanted to add the theme (as we chatted about on Twitter), but noticed it looked funny with a default background...so I made it an additional option you can set when rendering prompt examples.

If this isn't a wanted change/direction, no sweat at all - I can tweak the submitted theme to be more in line with a darker BG instead. I was mostly curious and went down the rabbit hole of making it configurable in case that was agreeable though.

I wasn't sure how contributor additions work either, but wanted to make that as smooth as possible so tried to do all the work here. Hope this is useful, and again no worries at all if it isn't, had fun editing and I do very much appreciate the wonderful project :)

Here's an example render of the craver theme using these options (it should auto-generate this if I understand the build scripts correctly):
craver

I want to fix the default \u279C rendering on the 2nd line of the prompt (an issue in many themes), but that's a font issue. I'll try and hunt that down in a follow-up PR if possible & wanted.

Thanks for the awesome project and the ask if this config could be included <3

I wanted to add the theme (as we chatted about on Twitter), but noticed it looked funny with a default background...so I made it an additional option you can set when rendering prompt examples.

If this isn't a wanted change/direction, no sweat at all - I can tweak the submitted theme to be more in line with a darker BG instead. I was mostly curious and went down the rabbit hole of making it configurable in case that was agreeable though.

I wasn't sure how contributor additions work either, but wanted to make that as smooth as possible so tried to do all the work here. Hope this is useful, and again no worries at all if it isn't, had fun editing and I do very much appreciate the wonderful project :)
@NickCraver
Copy link
Contributor Author

Adding context, an actual prompt render looks very close like this (I was hacking the renderer to make this work):
craver-theme

Things I think could use love from poking here:

  1. Some glyph issues (all font, I think)
  2. Execution Time isn't included (guessing this is more fundamental to how that works)

...so a bit of work left to do to make it more accurate. I'll try and poke at this some more!

@NickCraver
Copy link
Contributor Author

NickCraver commented Sep 17, 2021

On the first one above: updating the fonts (from here) in src/font to latest gets the glyphs rnedering, but the .MeasureString() calls down in GG don't work the same way PowerShell does. Here's how PowerShell renders, reducing the size of the glyph:
image

Compare to the termshot code, rendering like this, and truncating the characters:
craver

If I were to manually extend width of the characters (here) as an example test:

		w, h := dc.MeasureString(str)
		if (str == "" || str == "" || str == "" || str == "") {
			w *= 2;
		}

...then we can see it is rendering them correctly, just stomping over their second half with the next character in the example above:

craver
(forget the overflow - byproduct of the hack here)

I'm not sure what the mechanism should do here...we could figure out if it's a double width glyph in some way and double width (needs to be accounted for in overall size too), but that only fixes the visual glitch, it still wouldn't accurately portray PowerShell's actual rendering. I don't have other OSes to test it on and see how it behaves cross-platform though.

Anyone have more info/suggestions here? Trying to at least get to the bottom of what's happening :)

Copy link
Owner

@JanDeDobbeleer JanDeDobbeleer left a comment

Choose a reason for hiding this comment

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

Amazing work, this a good addition! I would only split the theme addition and code change so that we have a clean, contextual history:

  • theme: add Craver
  • feat: image background color

And now that I think about it, the background addition would also need this added to the docs and Powershell module.

@NickCraver NickCraver changed the title Themes: add Craver and add background colors Themes: add option for image background color Sep 17, 2021
@NickCraver
Copy link
Contributor Author

@JanDeDobbeleer Thanks for the review! If updates are good (hope I got everything - will tweak more if not!) this one should be feature only if you're good with squashing for a clean history :) Let me know if not, happy to adjust! Since the craver theme depends on this, can open the other PR as a follow-up once we're good.

@JanDeDobbeleer JanDeDobbeleer enabled auto-merge (squash) September 17, 2021 16:09
@NickCraver
Copy link
Contributor Author

@JanDeDobbeleer Interesting - I didn't do full commit messages in the rollback anticipating a squash...but those commits block the squash, heh

@JanDeDobbeleer
Copy link
Owner

@NickCraver relax. I can overrule that 😉

@NickCraver
Copy link
Contributor Author

Ah awesome, was about to collapse down into one and force it up if not :)

@JanDeDobbeleer
Copy link
Owner

@NickCraver I only need to be at home, the app doesn't allow that yet. I'll get it merged today 👍🏻

@JanDeDobbeleer JanDeDobbeleer merged commit bf51f59 into JanDeDobbeleer:main Sep 17, 2021
NickCraver added a commit to NickCraver/oh-my-posh that referenced this pull request Sep 17, 2021
This fixes the background color to actually work, and quotes author as well (first usage of the field introduced in JanDeDobbeleer#964.
JanDeDobbeleer pushed a commit that referenced this pull request Sep 17, 2021
this fixes the background color to actually work, and quotes author as well (first usage of the field introduced in #964)
@@ -69,6 +70,7 @@ themeConfigOverrrides.set('zash.omp.json', newThemeConfig(40, 40));
let poshCommand = `oh-my-posh --config=${configPath} --shell shell --export-png`;
poshCommand += ` --rprompt-offset=${config.rpromptOffset}`;
poshCommand += ` --cursor-padding=${config.cursorPadding}`;
poshCommand += ` --bg-color=${config.cursorPadding}`;

Choose a reason for hiding this comment

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

I think here is a bug. Should it not be --bg-color=${config.bgColor}?

Copy link
Owner

Choose a reason for hiding this comment

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

That's been fixed in a later PR.

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.

None yet

3 participants