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 zsh terminal prompt support #722

Merged
merged 3 commits into from
Jul 16, 2020
Merged

Conversation

nilason
Copy link
Contributor

@nilason nilason commented Jun 19, 2020

This addresses #719, regarding missing GRASS specific terminal prompt with zsh.

The code is more-or-less a copy of bash_startup() with bash replaced with zsh at appropriate places and adopted PS1 to zsh.

This seems to work as is, but zsh_startup() might be merged with bash_startup() using variables and I'm not familiar with every part of the code (and how to test it) – I therefore leave this as WIP.

For testing this PR : do chsh -s $(which zsh) and open new terminal.

@nilason
Copy link
Contributor Author

nilason commented Jun 25, 2020

I have tested every bit I could think of: reading from ~/.grass.zshrc and ~/.grass7/zshrc, copying aliases from ~/.alias, running grass_prompt and with this update of this PR I believe this is good to go.

I have seen there has been discussions on reading alias trac 4007 and in comments on #170 –– I took the liberty to "hardcode" the path to ~/.alias. I realise this may not be a good solution for a multiuser environment (with which I have no experience of with GRASS), but there are other hardcoded settings in $LOCATION/.zshrc such as HOME and PATH...

@nilason nilason changed the title [WIP] Add zsh terminal prompt support Add zsh terminal prompt support Jun 25, 2020
@nilason
Copy link
Contributor Author

nilason commented Jun 25, 2020

Come to think of it: the $LOCATION/.zshrc is (re-)written at every startup, so the multiuser scenario mentioned above isn't an issue. I cannot think of any reason not to expand the path to ~/.alias.

@nilason
Copy link
Contributor Author

nilason commented Jul 3, 2020

Latest update will produce a result like:

grass79
Starting GRASS GIS...
...
Mapset <PERMANENT> in Location <nc_spm_08_grass7>                               
GRASS 7.9.dev : ~ > r.mask raster=aspect
All subsequent raster operations will be limited to the MASK area. Removing
or renaming raster map named 'MASK' will restore raster operations to
normal.
Mapset <PERMANENT> in Location <nc_spm_08_grass7>                               
GRASS 7.9.dev : ~ > g.mapset user1                        [Raster MASK present]
Mapset switched. Your shell continues to use the history for the old mapset
Mapset <user1> in Location <nc_spm_08_grass7>                                   
GRASS 7.9.dev : ~ > g.mapset PERMANENT
Mapset switched. Your shell continues to use the history for the old mapset
Mapset <PERMANENT> in Location <nc_spm_08_grass7>                               
GRASS 7.9.dev : ~ > r.mask -r                             [Raster MASK present]
Raster MASK removed
Mapset <PERMANENT> in Location <nc_spm_08_grass7>                               
GRASS 7.9.dev : ~ > 

I can't help myself fiddling with this, would appreciate if someone took a look at this and put an end to it 😄
Jokes aside, zsh is with macOS 10.15 the default shell so this is a needed improvement.

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

To make zsh fully supported, you can also add it to g.mapset to provide a proper "fix history message":

https://github.com/OSGeo/grass/blob/master/general/g.mapset/main.c#L208

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

The visual result is up to you, so if this what you prefer or what is technically possible, I'm fine with that.

About the similarities with Bash, I'm not comfortable with the large overlap, but I don't have any good solution to account for the tiny differences all over the place.

As for the behavior with rc files and aliases, sourcing sounds good, but I can't quite get the whole picture at this point reading again discussion in #170. Perhaps @HuidaeCho has a better idea.

But still, looking on the quotations in #170, have you tried not doing the HOME change hack? The statement there by Glynn Clements ("AFAICT, the reason for messing about with the shell startup was to force the use of per-mapset history files") suggests that if history is handled through other means (here HISTFILE), the HOME hack is not needed, doesn't it?

lib/init/grass.py Outdated Show resolved Hide resolved
lib/init/grass.py Outdated Show resolved Hide resolved
@HuidaeCho
Copy link
Member

About the similarities with Bash, I'm not comfortable with the large overlap, but I don't have any good solution to account for the tiny differences all over the place.

Just curious if there are major differences between bash and zsh to require separate but substantially overlapping code. Looking at zsh_startup(), I feel more than 50% of the code is copy-paste from bash_startup(). I would recommend to add a flag to bash_startup() and reuse the code instead of duplicating it. It will be much easier to maintain the code later.

As for the behavior with rc files and aliases, sourcing sounds good, but I can't quite get the whole picture at this point reading again discussion in #170. Perhaps @HuidaeCho has a better idea.

Currently, all this startup code is a little bit messy. For example, like @wenzeslaus mentioned in #2681, LOCATION vs. LOCATION_NAME. LOCATION is actually NOT the name of a location. It's very misleading.

For bash, I didn't source ~/.bashrc from the startup code and left it to the user. By simply adding source ~/.bashrc in ~/.grass7/bashrc, you can source this file if you want. If you want a cleaner environment, don't source it. That's a user choice.

But still, looking on the quotations in #170, have you tried not doing the HOME change hack? The statement there by Glynn Clements ("AFAICT, the reason for messing about with the shell startup was to force the use of per-mapset history files") suggests that if history is handled through other means (here HISTFILE), the HOME hack is not needed, doesn't it?

Exactly! It would be ideal to remove the HOME hack altogether. Why not just set HISTFILE to $MAPSET_PATH\.bash_history? We need to seriously revisit the startup code.

lib/init/grass.py Outdated Show resolved Hide resolved
@nilason
Copy link
Contributor Author

nilason commented Jul 6, 2020

Thanks @wenzeslaus and @HuidaeCho for taking time and effort to look into this.

As I mentioned in initial comment an option would be to merge the two functions into one as you both also mentioned. Now I did that in last push. Contrary to my fear, I believe this didn't lead to a too complicated code.
I strived to leave bash as it was. I only did source for zsh .alias, but this would be possible for bash as well (the present line: f.write("test -r ~/.alias && . ~/.alias\n") doesn't do anything as $HOME at this stage is in $LOCATION and there is no .alias file there).
I hesitated to remove $LOCATION variable, maybe something for GRASS 8? But if you think now is the time, I'll remove it.

Regarding avoiding the HOME hack. I have not yet tried it, but I'm confident it is feasible. Yet perhaps not a trivial one, better left for GRASS 8 I believe. My hope for this PR is to backport it to 7.8 as well (one reason I initially added a separate method for zsh). Apart from setting HISTFILE, measures has to done to contrive support for the various shells (for setting prompts etc.).

I changed the prompt for zsh to look a bit more like bash:

[Raster MASK present]  
GRASS 7.9.dev : ~ >                    Mapset <PERMANENT> in <nc_spm_08_grass7>

the mask info (the PROMPT_COMMAND) is only printed if set, as now is the case for bash. But I lifted the location from PS1 and added this to RPROMPT together with mapset. The reason for this is that the prompt can be very long with a long location name and this in particular when working directory is not home, and I miss the mapset.

I sneaked in a fix for trac issue 3009 (failure to quit GRASS from GUI on macOS and FreeBSD) adding f.write("trap \"exit\" TERM\n") . If you think this may cause problems on some platforms this has to be put in a if clause.

@nilason
Copy link
Contributor Author

nilason commented Jul 6, 2020

To make zsh fully supported, you can also add it to g.mapset to provide a proper "fix history message":

Thanks for the reminder. Added.

@wenzeslaus wenzeslaus self-requested a review July 7, 2020 00:14
@nilason
Copy link
Contributor Author

nilason commented Jul 7, 2020

@lbartoletti It would be great if you could test this on FreeBSD, if the zsh prompt works at all, but also if the GUI "Quit GRASS" (see #408 for previous discussion) works (with bash or zsh).
(for testing this there's no need to recompile, only copy grass.py)

In case a condition will be needed, what result gives python command import sys; sys.platform on FreeBSD? I assume a check: isFreeBSD = 'freebsd' in sys.platform would be good.

lib/init/grass.py Outdated Show resolved Hide resolved
lib/init/grass.py Show resolved Hide resolved
lib/init/grass.py Show resolved Hide resolved
lib/init/grass.py Outdated Show resolved Hide resolved
lib/init/grass.py Outdated Show resolved Hide resolved
lib/init/grass.py Outdated Show resolved Hide resolved
@wenzeslaus
Copy link
Member

I suggest to move ahead with this. I agree that the alias. history, and fake home situation should be resolved and we made advancements in our understanding, but let's resolve that in another PR. Same for wrapping all shells functions into one (or whatever is what you are heading towards). It seems to me that original intention and necessary code refactoring are done.

This conflicts with #768 which I would like to merge, but it already conflicts with master branch, so I though it would be easier to merge this first and then resolve all conflicts in #768. Alternatively, I can merge #768 and you (@nilason) resolve the conflict here (should be mostly indentation).

@nilason
Copy link
Contributor Author

nilason commented Jul 15, 2020

I updated following suggestions from you both, as I deemed correct. Please do (re-)raise any remaining issue you might have.

Also, please do address the previously mentioned issue:

I sneaked in a fix for trac issue 3009 (failure to quit GRASS from GUI on macOS and FreeBSD) adding f.write("trap "exit" TERM\n") . If you think this may cause problems on some platforms this has to be put in a if clause.

This could be enclosed in something like:

if 'freebsd' in sys.platform or 'darwin' in sys.platform:

@wenzeslaus wenzeslaus merged commit c5a5e71 into OSGeo:master Jul 16, 2020
@wenzeslaus
Copy link
Member

Thanks @nilason . Bash and zsh work for me. The GUI exit works for me on Linux. History works only with bash, not with zsh. However, this is an improvement as it is, so I merged and will leave it on you or others to open new issues or PRs.

@nilason nilason deleted the fix_zsh_prompt branch July 16, 2020 06:02
@nilason
Copy link
Contributor Author

nilason commented Jul 16, 2020

@HuidaeCho and @wenzeslaus Thanks!

History works only with bash, not with zsh.

Thats the problem with not being able to test on different platforms. For a quick check @wenzeslaus, could you please run in a zsh terminal echo "HISTSIZE <$HISTSIZE> SAVEHIST <$SAVEHIST>, HISTFILE <$HISTFILE>" before and while running grass and compare the result with description on A User's Guide to the Z-Shell – 2.5.4: Setting up history.

@wenzeslaus
Copy link
Member

SAVEHIST is 0. Changing it to non-zero fixes the issue for me. Outside of the GRASS session, in zsh, it is 1000.

GRASS 7.9.dev : grass > echo "HISTSIZE <$HISTSIZE> SAVEHIST <$SAVEHIST>, HISTFILE <$HISTFILE>"           Mapset <PERMANENT> in <demolocation>
HISTSIZE <3000> SAVEHIST <0>, HISTFILE </home/.../PERMANENT/.zsh_history>

@nilason
Copy link
Contributor Author

nilason commented Jul 17, 2020

SAVEHIST is 0. Changing it to non-zero fixes the issue for me. Outside of the GRASS session, in zsh, it is 1000.

Thanks for checking! Suspected something like that. Shouldn't be too complicated to fix. Will put a new PR.

@neteler
Copy link
Member

neteler commented Jul 30, 2020

Shall this be backported, @nilason ?

@nilason
Copy link
Contributor Author

nilason commented Jul 30, 2020

That would be preferable :), zsh is the default mac terminal shell from latest OS version (10.15). This might have side effect on bash too, but is on master for two weeks now, so maybe safe. In any case, a backport label could do for now.

neteler pushed a commit that referenced this pull request Jul 30, 2020
Adds zsh terminal prompt support and includes history change message for zsh in g.mapset.

Fixes #719.
@neteler neteler added this to the 7.8.4 milestone Jul 30, 2020
@neteler
Copy link
Member

neteler commented Jul 30, 2020

I have backported it, please test it.

@nilason
Copy link
Contributor Author

nilason commented Jul 30, 2020

I have backported it, please test it.

Great, thanks! Will test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Shells
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants