init: Change the history file according to the current mapset for Bash #930
Conversation
Bash now saves the history every after every command removing the need to write history before or when the mapset is switched. Changing HISTFILE (shell) variable makes Bash write to a different file, so contructing the path each time from mapset path and hardcoded history file name. Also increasing the history size slightly.
Given the implemenation of GConsoleWindow, even when the prompt/interactive console is hidden, giface needs to be passed, so the change needs to be present everywhere when GConsoleWindow is used. Direct usages of GPromptSTC could avoid the updates if history is not needed, but that's not implemented.
There was a problem hiding this comment.
It works great in the terminal. In g.gui, commands in the Console get written to the history file and shared with the terminal when the mapset is switched. However, the GUI Console stays the same after a mapset switch. Terminal history is not shared with the GUI Console while GUI history is shared with the terminal when switching the mapset. It can be confusing when you switch between the GUI and terminal. Maybe, we have to consider these two command line interfaces separately. OR the GUI Console needs to reread the history file on switching mapsets both from the terminal or GUI itself. Currently, it seems to read the history file only when the GUI starts.
Even in just the terminal, it's confusing when you switch mapsets. It'll take some getting used to.
echo hello
g.mapset mapset2
<UP><UP> # shows the previous history of mapset2, not echo hello, as designed
|
Thanks for thoroughly testing it! It is somewhat convoluted and I think you expect some other behavior than I do. I updated the PR description to clarify more and formulated that as a bug for GUI+terminal combo (and an enhancement for terminal only).
The history in GUI gets written after every command is executed (even before this PR). In terminal, only after mapset change is detected when getting a fresh prompt, the history is recorded to the history file. In terminal, that's also when the mapset history file is read into the history list (the history you browse interactively). In GUI, however, the history list is updated from the mapset history file only when the mapset change was done from the GUI (current GUI limitation affecting only people who change mapset from terminal, i.e., advanced users; something to be resolved anyway). I don't understand what you mean by "sharing" and there is a difference between switching from GUI and from terminal, so if you could rephrase it taking into account what I wrote above, that would be great.
I think this is a great idea. The current GUI code always uses .bash_history regardless of SHELL or platform which now more relevant when macOS switched to Z shell from Bash - users will see change in behavior due to change in the default shell. Users who always used something else then Bash always had the behavior with the separate histories. Different shells use different history files. wxGUI (or the Console there) is another "shell" with commands being issued therein. I think I would do a separate PR for that, but do you have any further thoughts on this? Suggestions for a name (.gui_history, .gui_console_history, .wxgui_history)?
Read the history file only when the GUI starts is exactly the old behavior this PR is trying to fix. Did you compile your GUI code (this PR is spread over 3 places in the source code)? Or is this related only to the issue of GUI not reading it when the change was done in the terminal?
This is the part where you expect something else then I do, so that would be good to discuss more. This is exactly the behavior I expect because it is what the original message in g.mapset suggests (perhaps with the additional assumption that you don't want to mix history of mapset1 into mapset2 when the session ends) and, perhaps more importantly, it is what you get when you turn off GRASS GIS in mapset1 and start again in mapset2. If "echo hello" would be "r.info xy" but xy is not on path in the current mapset's search path, do you still expect to have it there in history list and in the history file once the history gets written? It may just depend on one's workflow, you may want to execute the previous command in the mapset2, but you may also want to pick up where you left in mapset2. Perhaps these two use cases are what we are speaking about? For this (and perhaps also for the separate history file for the GUI), the ultimate question is what the history actually represents or what is its purpose? All the commands executed by the user (executed by direct input, although though different means)? Something you could use reconstruct to your workflow or provenance of the data in the mapset? Simply my last used stuff? Something in between? Why we have different histories for different mapsets anyway? |
|
Just some thoughts about the different histories for different mapsets question:
All in all, I think keeping separate history files for different mapsets is a positive feature |
Let's follow this in #962. |
|
Thanks for listing these. It is a good confirmation we are on the right track.
The history is attached to a mapset as opposed to one history for user which would be the default behavior for Bash and other shells. Each user has their mapset thus their own history.
They need to know what shell you used to look at the file unless they happen to use the same one or the GUI. It will also be possibly spread in multiple files spread on macOS and on Linux after #962. The files will be hidden by default on unix, although we could change that to not keep the dot in the names.
Right, the choice is really only between: one for each user intermingled with your other history or separate and one for each mapset.
I'm using it myself a lot. It's just more complicated then one file with all history (there is a size limit set too). Hence this discussion and my questions. |
The standalone forms were not using giface, None was passed instead which caused some parts relying on giface to be there to fail, specifically, the disabled prompt since 2d24a42 (OSGeo#930). The prompt is not used in forms, but it is not decoupled from GConsoleWindow, so disabling it, only hides it. This required to correctly handle implementation of StandaloneGrassInterface::UpdateCmdHistory() which was not called previosly because the code relied to giface to not be there in the standalone case instead of dealing with the implementation differences through the interface. Now the StandaloneGrassInterface implements this function as no-op since there is nothing to do here. No interactive prompt to update. The giface function UpdateCmdHistory() was not previously not defined in terms of what it does. This is now fixed and the new implementation in StandaloneGrassInterface makes sense in light of this definition (which is based on the implemenation of this method in lmgr). The function was missing from the abstract interface class which served so far mainly as the general interface documentation. This is now fixed and StandaloneGrassInterface now newly inherits from this class. The inheritance is optional, but here it can provide useful default implementations for the functions which are not supported by (and should not with) the StandaloneGrassInterface instances. This fixes the standalone forms not starting (from command line) after 2d24a42.
…#971) The standalone forms were not using giface, None was passed instead which caused some parts relying on giface to be there to fail, specifically, the disabled prompt since 2d24a42 (#930). The prompt is not used in forms, but it is not decoupled from GConsoleWindow, so disabling it, only hides it. This required to correctly handle implementation of StandaloneGrassInterface::UpdateCmdHistory() from 22597e6 which was not called previously because the code relied on giface not being there in the standalone case instead of dealing with the implementation differences through the interface. Now the StandaloneGrassInterface implements this function as no-op since there is nothing to do here. No interactive prompt to update. The giface function UpdateCmdHistory() was not previously not defined in terms of what it does. This is now fixed and the new implementation in StandaloneGrassInterface makes sense in light of this definition (which is based on the implemenation of this method in lmgr). The function was missing from the abstract interface class which served so far mainly as the general interface documentation. This is now fixed and StandaloneGrassInterface now newly inherits from this class. The inheritance is optional, but here it can provide useful default implementations for the functions which are not supported by (and should not with) the StandaloneGrassInterface instances. This fixes the standalone forms not starting (from command line) after 2d24a42.
Describe the bug
In the system terminal (Bash shell), after switching the mapset using g.mapset, the history continues to be written to the previous mapset. g.mapset warns about it. A nice enhancement would be if the change of history suggested in the message would happen automatically.
However, since now with the new startup mechanism in GUI, an expected usage is starting the GUI and then possibly changing the mapset from there and then working in command line (or GUI), the message about history is not displayed and if displayed, it would confusing to the user and be somewhat disconnected with the terminal anyway. Hence, this is a bug: The shell (nor the Console in GUI) does not reflect change of the mapset by switching the history to the current mapset.
To Reproduce
Steps to reproduce the behavior:
Expected behavior
When I switch mapset in the GUI after starting GRASS GIS, the shell and also the Console in GUI need to reflect it in terms of showing the current mapset history and saving commands to the current mapset history file.
Using the history of that mapset switched to is expected behavior because that's how GRASS GIS behaves when given the path to mapset in the command line.
System description (please complete the following information)
Additional context
This issue is more prominent after the revamp of the GUI startup when the mapset switch may more likely happen from an existing session.
The PR #923 addressed the issue of no update of location name in the prompt (#921).
This issue might be reduced if the terminal window is not shown to the user unless asked for, e.g., by starting from existing terminal window, i.e., start shell only when starting from shell. (This is a distribution issue now. GRASS GIS supports both options since #768.)
It seems it is might not be clear what is expected behavior here: when history should stay the same and when it should change?
Fix in this PR
With this PR, the history file and history list (the in-memory history browsed, e.g., by arrows) are now updated when the current mapset is changed for Bash.
In the shell, it does not matter how the mapset was switched. Once the prompt is updated (by finishing g.mapset command or by pressing Enter or Ctrl+C), the previous mapset history file is updated and the history list is loaded from the current mapset history file. The update happens only when needed, i.e., when the mapset was actually switched, not for every prompt update which is important with large history files (a slight delay of prompt would be otherwise noticeable with 5000 lines in the history file). The standard writing to the history file happens at the end of the session which is the state before this PR.
g.mapset is updated so that it does not show the "fix your history things" message for Bash anymore and only says "mapset changed" which is the core of the message without the warning about the history. The important message level is kept.
The GUI now updates the "history list" in the Console/Prompt object as well when the change happens in the GUI. The writing to the file is done at a different place, for each command separately always using the current mapset. The change in GUI spreads to more places than desired, but the Prompt is present in several places although sometimes hidden.
Limitations and scope of this PR
This PR is only for Bash. Z shell and tcsh are left untouched. Other shells don't have any special history handling.
GUI always uses
.bash_historyregardless of theSHELLvariable.The GUI is not detecting the change of the mapset from the shell. This PR is leaving it as it is as it is focused on fixing start with GUI and use command line later and further it is enhancing the case when command line is used as the primary or only tool. Given the current GUI code, the change of mapset in the shell won't influence the GUI immediately, so the history, specifically the history browsed by Arrow Up, won't be updated. (The executed commands are already always saved to current mapset, so the issue is only reading/using the history, not adding/writing to it.) When the detection is fixed in GUI, the mechanism implemented in this PR will kick in.