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

VCS symbols #298

Merged
merged 9 commits into from
Jan 15, 2018
Merged

VCS symbols #298

merged 9 commits into from
Jan 15, 2018

Conversation

emansije
Copy link
Contributor

@emansije emansije commented Oct 4, 2017

Taking on @mislam's PR #170, I adapted it to the current state of the VCS' segments.

In the meanwhile I've noticed that the svn segment wasn't working properly and was implemented as a basic segment, so I decided to reimplement it as a threaded one. I should have used a separate branch for this but figured that it wouldn't be worth the trouble. I'll probably implement the svn tests later on, though, and for that I'll use a separate branch + PR, of course.

@emansije
Copy link
Contributor Author

emansije commented Oct 5, 2017

Forgot to mention:

The symbols I used for git, hg and svn were the same I've seen in other projects to represent those VCS. For bzr I used \u25C8 (◈) which seemed to resemble somewhat the official logo:

(at least with the font I'm using) and for fossil I used \u24BB which is simply F in a circle (Ⓕ), for lack of a better option. This is the official logo:

The choice of symbols is open for debate, of course.

@emansije
Copy link
Contributor Author

emansije commented Oct 5, 2017

Maybe a simple \u25C7 (WHITE DIAMOND) (◇) would be a better choice for bzr... or even, perhaps, the combination \u2B61\u20DF:
imagem

SVN implements the branch notion at server level and the working copies don't
hold that information. This implementation gets the working copy root path and
extracts the basename, using it as a local branch name.
@emansije
Copy link
Contributor Author

emansije commented Oct 7, 2017

The only symbols that I find minimally adequate to represent a fossil repository are \u2332 (CONICAL TAPER) (⌲), because it looks a bit like the fossil's logo head, \u1F480 (SKULL) (💀), \u1F571 (BLACK SKULL AND CROSSBONES) (🕱), \u1F41C (ANT) (🐜) and \u1F43E (PAW PRINTS) (🐾). These last four can relate to fossils, but it might be a bit of a stretch. Seeing them in use:
imagem
imagem
imagem
imagem
imagem

Skull symbols would probably strike as macabre or violent to some people, so it might not be a good choice. Paw prints would be of a sabre-tooth cat and I guess most people wouldn't easily relate them to fossils. I'd personally choose the conical taper or the ant. Any thoughts?

@emansije
Copy link
Contributor Author

Any thoughts about this PR?

@b-ryan
Copy link
Owner

b-ryan commented Oct 30, 2017

Hey @emansije - I have just been swamped lately. Sorry for no progress on this. I am starting to think the symbols should be moved into the "themes" - in fact I think the separators could go in there as well. The default VCS symbols could just be empty to preserve backwards compatibility and then maybe we could just provide instructions for a recommended set of default symbols to use for VCS segments.

@emansije
Copy link
Contributor Author

Hey @emansije - I have just been swamped lately. Sorry for no progress on this.

No problem, I totally understand.

I am starting to think the symbols should be moved into the "themes" - in fact I think the separators could go in there as well.

It would make it easier and cleaner to customize, indeed.

The default VCS symbols could just be empty to preserve backwards compatibility and then maybe we could just provide instructions for a recommended set of default symbols to use for VCS segments.

Wouldn't it be better leave the current symbols as default? It seems to me that it would be nicer as an out-of-the-box experience.

@b-ryan
Copy link
Owner

b-ryan commented Nov 6, 2017

It seems to me that it would be nicer as an out-of-the-box experience.

That depends who you ask. For example, I don't want the symbols. I much prefer to strip out anything that doesn't provide me real value. And for me, the symbols provide only aesthetic value.

So it sort of depends whether the majority of users would want them. Although I can see the argument for leaving them in, since I think it will be easier from a documentation point of view to say "this is how you strip out these things" vs "there are these other things you can add to your prompt if you want."

@emansije
Copy link
Contributor Author

emansije commented Nov 6, 2017

It seems to me that it would be nicer as an out-of-the-box experience.

That depends who you ask. For example, I don't want the symbols. I much prefer to strip out anything that doesn't provide me real value. And for me, the symbols provide only aesthetic value.

Sorry, my bad. The way that I phrased it, it suggests that this PR will automatically show VCS symbols. I totally agree that VCS symbols shouldn't be shown by default and I made sure that was the case in the absence of explicit configuration, so it remains backwards compatible. But my point was that, in case the user wants to have those symbols showing, switching them on in the configuration will be enough to get a standard set of symbols.

I'd say that it would be better to merge this PR to make a starting point for the migration into the themes. What do you think?

@b-ryan
Copy link
Owner

b-ryan commented Dec 12, 2017

I'd say that it would be better to merge this PR to make a starting point for the migration into the themes. What do you think?

The problem I have with that is breaking backwards compatibility. Seems like it would be better to not require users to switch things when they change. Especially since I don't think it would be a huge amount of work to implement as a theme option, right?

@emansije
Copy link
Contributor Author

Sorry, but I'm a bit lost here. In what way does it brake backwards compatibility? You mentioned that the VCS symbols should be empty by default and I clarified that I made sure that they were empty unless the user explicitly declared in the configuration the intention to show them. Am I misunderstanding something?

As for moving VCS symbols into themes, wouldn't that make it less flexible? I mean, would a user be forced to select a particular theme that included this feature or are you thinking about implementing themes in some sort of modular and cumulative way?

I much prefer to strip out anything that doesn't provide me real value. And for me, the symbols provide only aesthetic value.

Well... I personally see the value in having identifying symbols. I work on some projects that are quite similar to each other but use repositories implemented with distinct VCS and the visual cue comes in handy.

@b-ryan
Copy link
Owner

b-ryan commented Dec 13, 2017

Yeah I was not entirely clear. My point was that if we go the route of using a config variable, then go ahead and switch to using a theme, at that point it would be a breaking change since the show_symbol option would no longer be used.

@emansije
Copy link
Contributor Author

OK, I get it now. But how would we move this into a theme and make it work with the rest of the themes? Do we ask the user to select the VCS symbols theme and modify it in order to import the preferred color theme? It seems a bit more cumbersome than having an option in the configuration file.

@b-ryan
Copy link
Owner

b-ryan commented Dec 14, 2017

You're definitely right about it being more cumbersome. I suppose a hybrid approach might be the best way after all, which is probably what you had in mind. Ie. put the symbols in the theme, but use a config flag to turn them on or off.

That approach does have a sort of slippery slope aspect to it - like there are quite a few symbols used throughout the code, if we were to think about those symbols like we are thinking about the ones in this PR, that would lead to too many config options.

I have a lot of thoughts running through my head. The one that is making the most sense for now is go your route and stick with what's in the PR. If you want to resolve conflicts I can get that going.

@emansije
Copy link
Contributor Author

(...) which is probably what you had in mind.

Indeed.

That approach does have a sort of slippery slope aspect to it - like there are quite a few symbols used throughout the code, if we were to think about those symbols like we are thinking about the ones in this PR, that would lead to too many config options.

It's a good point. I've been thinking about that and haven't come up with a good solution yet.

If you want to resolve conflicts I can get that going.

Done. I'm already sketching the theme version in my head.

@b-ryan b-ryan merged commit bbaacc1 into b-ryan:master Jan 15, 2018
@b-ryan
Copy link
Owner

b-ryan commented Jan 15, 2018

I had to fix the svn segment since it overwrote recent changes by @kc9jud

Otherwise sorry it took so long!

@emansije
Copy link
Contributor Author

Thanks!

Actually, @kc9jud's version is broken in more than one way, like the fact that it doesn't take into account svn's localized output. I meant to comment his implementation, since my version in this branch was already fixed, but kept postponing (mainly because I was busy) and ended up forgetting to do it. But it's no biggie, I'll just open a new PR for that.

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