Skip to content
This repository has been archived by the owner on Apr 24, 2020. It is now read-only.

BenoitAverty's dir Segment Improvements #353

Closed
wants to merge 10 commits into from

Conversation

bhilburn
Copy link
Member

@bhilburn bhilburn commented Dec 9, 2016

Hey @BenoitAverty! These are some pretty nice improvements to the dir segment, and we think some of them might be great contributions to the upstream! Creating this PR to get the discussion rolling.

@bhilburn bhilburn added this to the Release v0.6.0 milestone Dec 9, 2016
@bhilburn bhilburn self-assigned this Dec 9, 2016
@bhilburn bhilburn requested a review from dritter December 9, 2016 16:35
if [[ $POWERLEVEL9K_DIR_SPLIT_MODE == true ]]; then
current_path=$(print -P $current_path)
if [[ $current_path == "/" ]]; then
current_path=$(print_icon 'ICON_SLASH')
Copy link
Member

Choose a reason for hiding this comment

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

The icon is missing in the icons array.
And may I suggest a new name for that. I see that ROOT_ICON is already in use (and has a different meaning anyway), but what about ROOT_FOLDER_ICON?

if [[ $current_path == "/" ]]; then
current_path=$(print_icon 'ICON_SLASH')
else
current_path=$(echo $current_path | cut -c1 | sed "s,/,$(print_icon 'ICON_SLASH') ,g")$(echo $current_path | cut -c2- | sed "s,/, $(print_icon 'LEFT_SUBSEGMENT_SEPARATOR') ,g")
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, you are replacing the first slash with ICON_SLASH and all following slashes with LEFT_SUBSEGMENT_SEPARATOR. I recently implemented such a functionality (see #346 ; but without a different icon for the first slash, which is a good idea, IMHO).

@@ -956,6 +985,13 @@ prompt_pyenv() {
fi
}

# dir_permision: Display information about the user's permission to write in the current directory
prompt_dir_permission() {
Copy link
Member

Choose a reason for hiding this comment

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

Here I would propose dir_writable as a better name.

@BenoitAverty
Copy link
Contributor

Hello,

I'd be glad to make these changes land into upstream and drop my own fork of powerlevel9k 😄

I'll try to find some time in the next few days to take your reviews into account and clean up a bit, rebase, etc.

Are you willing to integrate all the changes or only the changes regarding "dir" segment ?

Thanks for your interest in my changes :-)

@bhilburn
Copy link
Member Author

@BenoitAverty - I think all of your changes would be good upstream =)

Don't forget that to add docs in the README for the dir updates an the new truncation strategy!

@bhilburn
Copy link
Member Author

Hey @BenoitAverty - Just keeping this on your radar! We're excited to pull this into the upstream =)

@BenoitAverty
Copy link
Contributor

Hey @bhilburn. Sorry I haven't had time to work on this yet, but I haven't forgotten ^^

Just so I don't keep your hopes up, I probably won't have time to work on this before next year, with all the holidays and all.

It's on my todo list !

@dritter
Copy link
Member

dritter commented Jan 28, 2017

Hey @BenoitAverty !
Did you have time to polish your branch? I could need the upsearch function in some other segments. :)

@BenoitAverty
Copy link
Contributor

Hello @dritter

I'm really sorry, I don't see this happening soon. I'm having troubles finding the time and motivation to do it at the moment, and it would probably take a few hours.

If someone wants to do it, I don't mind answering questions, and I have no problem letting someone take credit for this. They would need to pick the changes I made from the git history and do it cleanly.

Benoit

@dritter
Copy link
Member

dritter commented Jan 31, 2017

@BenoitAverty Thanks for the update. Don't worry, I'll take over your PR in the next couple of days ;)

@dritter dritter self-assigned this Feb 1, 2017
@dritter
Copy link
Member

dritter commented Feb 7, 2017

Closing this now, as the work is continued in #398 and #395

@dritter dritter closed this Feb 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants