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

Add git stash segment #379

Merged
merged 3 commits into from
Apr 10, 2018
Merged

Add git stash segment #379

merged 3 commits into from
Apr 10, 2018

Conversation

apinkney97
Copy link
Contributor

Up-to-date rework of #201, addressing issue #114.

This also refactors some common functions from the version control segments into utils.py.

@b-ryan
Copy link
Owner

b-ryan commented Mar 20, 2018

Thanks for updating this. The diff shows a lot of changes - I'm not sure why. Perhaps you have to merge the latest master?

@apinkney97
Copy link
Contributor Author

I think it is from the latest master (it's from ae33f7e at least).

This is broken into two commits:

  • fc62b0f touches a lot of files since it refactors out a lot of duplicated functions into utils.py
  • 709c5ba is the commit that adds the git stash segment, and uses the refactored functions

@uloco
Copy link

uloco commented Apr 9, 2018

Still waiting for this like crazy... 😞

@apinkney97
Copy link
Contributor Author

@b-ryan if you prefer I can rework this without fc62b0f (ie leave all the duplicated get_PATH and x_subprocess_env functions and either copy those functions from segments.git or import them) but my preference would be to leave it as it currently stands.

@b-ryan
Copy link
Owner

b-ryan commented Apr 9, 2018

Sorry things have been crazy. Looking at this now.

@b-ryan
Copy link
Owner

b-ryan commented Apr 9, 2018

Code looks good. The only thing that gives me pause is the symbol used for the segment. Unicode symbols... Not all fonts have support for them. It would be useful to know whether the https://github.com/powerline/fonts fonts support this symbol.

In any case, the symbol should be added to https://github.com/b-ryan/powerline-shell/blob/master/powerline_shell/utils.py#L17 rather than be a constant so that the symbol is configurable.

@apinkney97
Copy link
Contributor Author

Anecdotally it seems to render ok on my terminal in all fonts I've tried (10 or so different ones, powerline, non-powerline monospace, and proportional fonts). Possibly my terminal is being clever and pulling it from some font that does contain that glyph.

image

Seems to work even for totally crazy fonts that you'd never use in a terminal that don't support powerline:
image

Not quite sure whether I've done this right (I haven't really poked around inside fonts before) but I think this should list which fonts support a given unicode character:

$ for i in $(find /usr/share/fonts /usr/local/share/fonts -name "*.ttf"); do showttf $i 2>/dev/null | grep -i 'U+2398' && echo $i; done | grep "\.ttf" 
/usr/share/fonts/truetype/ancient-scripts/Symbola_hint.ttf                              
/usr/share/fonts/truetype/freefont/FreeSerif.ttf                                        
/usr/share/fonts/truetype/freefont/FreeMono.ttf                                         

Admittedly it's not many (3 .ttf files out of 177) but I think those all shipped with the OS (Ubuntu 16.04).

Again, this is pretty anecdotal, but it seemed to work ok for people who commented on the old PR: #201 (comment)

@b-ryan
Copy link
Owner

b-ryan commented Apr 10, 2018

Nice - thanks for looking into the fonts & making that change.

@b-ryan b-ryan merged commit 404d3ff into b-ryan:master Apr 10, 2018
@uloco
Copy link

uloco commented Apr 11, 2018

🎉 🎉 🎉 🎉
Thank you guys :)))

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