DRAFT: Update themes to use new Automatic History Management #1951

gaelicWizard · 2021-09-18T07:37:29Z

Description

This follows up on my history (#1940) branch to adopt the two new functions in all themes which had already included history management.

Closes #1595.

Motivation and Context

This PR adopts the new functions from #1940 and removes all use of the old function _save-and-reload-history(), but does not remove the function yet (in case anyone is using it somewhere out of tree...for the time being).

A future PR should remove all history-related activities from all the themes. I didn't do that here as I want to be as minimally-invasive as possible in this PR. At that time, it will need to be determined how to account for users who have not enabled plugin/history but who use a theme which triggers history -a...their history would after that future PR suddenly stop saving the way they're used to.

How Has This Been Tested?

All tests pass.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code follows the code style of this project.
If my change requires a change to the documentation, I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
I have added tests to cover my changes, and all the new and existing tests pass.

gaelicWizard · 2021-09-24T02:01:37Z

@davidpfarrell, this is "stacked" on my automatic history management PR, which has been approved, but I'd love to start getting feedback on this one pending that one getting merged.

Do you think I should just remove all history management from themes rather than just adopting the new functions? I think that might require an update to the documentation too.

First pass to use _Bash It_'s automatic history management.

Use `preexec` and `$HISTCONTROL` to configure _Bash It_'s automatic history management feature.

gaelicWizard · 2022-02-20T20:28:54Z

@cornfeedhobo, would you suggest just yanking all history-related tomfoolery from themes or to update them to interact with the new automatic history management? Long term, I vote for themes-don't-touch-history, but I'm conflicted about short-term.

seefood · 2024-11-19T06:56:10Z

I'm all for a hook mechanism, of course, where themes don't touch any other function like history.
Could you just merge the conflicts? I was not able to figure out how to unbreak it correctly.

seefood · 2025-05-08T11:16:11Z

@akinomyoga looks legit?

akinomyoga

The description of thsi PR seems to imply that this PR is intended to preserve the existing behavior of themes while using the new mechanism of the history syncing. However, the changes in this PR seem to drop the support for HISTORY_AUTOSAVE, which the old mechanism used but lib/history.bash doesn't reference.

The following values of HISTCONTROL do not seem to be documented at all. Also, it should be clarified that auto-save is called before running the command, and auto-load is called after running the command. These should be documented somewhere.

From the PR cover of #1940 (comment)

autoshare: append to $HISTFILE when _bash_it_history_auto_save() called, and read from $HISTFILE when _bash_it_history_auto_load() called.

autosave: append to $HISTFILE when _bash_it_history_auto_save() called, but do not do anything when _bash_it_history_auto_load() called.

autoload: do not do anything when _bash_it_history_auto_save() called, but do read from $HISTFILE when _bash_it_history_auto_load() called.

noauto (default): don't do anything.

themes/nwinkler/nwinkler.theme.bash

themes/nwinkler_random_colors/nwinkler_random_colors.theme.bash

akinomyoga · 2025-05-08T11:36:38Z

themes/pete/pete.theme.bash

+		# Append new history lines to history file
+		HISTCONTROL="${HISTCONTROL:-}${HISTCONTROL:+:}autosave"
+		;;
+esac


Hooks into preexec and prompt_command are missing.

All the themes touched in this PR seems to have called _save-and-reload-history in prompt_command, but auto-load/save are registered to preexec/prompt_command for some themes and not for other themes. What are the differences? Are they just oversights? Or is it intentional?

edit: Anyway, I suggest removing all these additional hooks as discussed in #1951 (comment).

question is do I keep fixing this or just close and bury it? Maybe it's just the wrong approach altogether.

themes/powerline-plain/powerline-plain.base.bash

themes/rainbowbrite/rainbowbrite.theme.bash

akinomyoga · 2025-05-08T11:41:44Z

themes/codeword/codeword.theme.bash

+safe_append_preexec '_bash-it-history-auto-load'
+safe_append_prompt_command '_bash-it-history-auto-save'


Compared to _bash-it-history-init (in lib/history.bash), the commands for preexec and prompt_command are reversed.

bash-it/lib/history.bash

Lines 5 to 8 in dc25be3

function _bash-it-history-init() {

safe_append_preexec '_bash-it-history-auto-save'

safe_append_prompt_command '_bash-it-history-auto-load'

}

Thinking of the functions that auto-load and auto-save perform, I think the "save" should be performed before running the command, and the "load" should be performed before the prompt command.

In addition, why don't they simply call _bash-it-history-init?

Hmm, it seems _bash-it-history-init is anyway forcibly loaded at the end of bash_it.sh through _bash_it_library_finalize_hook.

bash-it/lib/history.bash

Line 49 in dc25be3

_bash_it_library_finalize_hook+=('_bash-it-history-init')

One possibility is that the above "reversed hooks" are intended to complement load/save that haven't been registered by _bash-it-history-init to perform both load/save in both preexec and prompt_command. However, I don't see the reason for running both twice before running the command and after running the command. The default hooks set up by _bash-it-history-init seem sufficient to me.

My suggestion is to remove all safe_append_preexec '_bash-it-history-auto-load' and safe_append_prompt_command '_bash-it-history-auto-save' from this PR.

in other words, it really needs to be redone. I'll dump it.

akinomyoga · 2025-05-08T11:44:53Z

themes/doubletime/doubletime.theme.bash

+case $HISTCONTROL in
+*'auto'*)
+	: # Do nothing, already configured.
+	;;
+*)
+	# Append new history lines to history file
+	HISTCONTROL="${HISTCONTROL:-}${HISTCONTROL:+:}autosave"
+	;;
+esac
+safe_append_preexec '_bash-it-history-auto-load'
+safe_append_prompt_command '_bash-it-history-auto-save'


Also, there are too many repetitions of this code section. These lines should be factorized into an independent function (defined in e.g. lib/history.bash), and I think each theme should contain a single line of the call to the function.

themes/font/font.theme.bash

akinomyoga · 2025-05-08T12:20:28Z

This is not about this PR itself, but this is about the implementation detail of "the new automatic history management" that this PR relies on:

bash-it/lib/history.bash

Lines 27 to 32 in dc25be3

    
           case $HISTCONTROL in 
        
           	*'noauto'*) 
        
           		: # Do nothing, as configured. 
        
           		;; 
        
           	*'autosave'*) 
        
           		# Append new history from this session to the $HISTFILE

I think the above case-statement should test the exact fields split by : instead of matching substrings. To do that one can use the following idiom:

case :$HISTCONTROL: in
 	*:'noauto':*)
 		: # Do nothing, as configured.
 		;;
 	*:'autosave':*)
 		# Append new history from this session to the $HISTFILE
[...]

gaelicWizard changed the title ~~DRAFT: Update themes to use new Automatic History Management~~ Update themes to use new Automatic History Management Sep 18, 2021

gaelicWizard force-pushed the history_themes branch from 0ae7dcb to 8220aa2 Compare September 20, 2021 19:50

gaelicWizard force-pushed the history_themes branch 2 times, most recently from edba5dd to 4ae213a Compare September 24, 2021 20:20

ira-bv mentioned this pull request Oct 11, 2021

Add history flushes on each command to the powerline themes #1883

Merged

3 tasks

gaelicWizard mentioned this pull request Dec 27, 2021

Feature: automatic history management #1940

Merged

6 tasks

gaelicWizard force-pushed the history_themes branch 13 times, most recently from 34c9d79 to 95148bf Compare January 2, 2022 04:58

gaelicWizard force-pushed the history_themes branch 5 times, most recently from 1c5a6d3 to ec96c60 Compare January 10, 2022 07:26

gaelicWizard force-pushed the history_themes branch 4 times, most recently from 8ee61fd to 455bbe2 Compare January 20, 2022 11:48

gaelicWizard force-pushed the history_themes branch from 455bbe2 to 52bac07 Compare January 23, 2022 01:04

gaelicWizard force-pushed the history_themes branch 5 times, most recently from d698955 to 0e88fd0 Compare January 30, 2022 06:06

gaelicWizard force-pushed the history_themes branch from 0e88fd0 to 7825eb9 Compare February 8, 2022 23:58

gaelicWizard and others added 9 commits February 8, 2022 16:59

themes: use _bash_it_history_auto_save()

d9b29b3

First pass to use _Bash It_'s automatic history management.

themes/font: adopt automatic history

0758b86

Use `preexec` and `$HISTCONTROL` to configure _Bash It_'s automatic history management feature.

theme/barbuk: adopt automatic history

a8f2dc5

Use `preexec` and `$HISTCONTROL` to configure _Bash It_'s automatic history management feature.

theme/rainbobrite: adopt automatic history

8a3cc89

Use `preexec` and `$HISTCONTROL` to configure _Bash It_'s automatic history management feature.

theme/powerline-plain: adopt automatic history

24a4067

Use `preexec` and `$HISTCONTROL` to configure _Bash It_'s automatic history management feature.

theme/pete: adopt automatic history

7823e36

Use `preexec` and `$HISTCONTROL` to configure _Bash It_'s automatic history management feature.

theme/nwinkler: adopt automatic history

569b8b8

Use `preexec` and `$HISTCONTROL` to configure _Bash It_'s automatic history management feature.

theme/doubletime: adopt automatic history

db04b2b

Use `preexec` and `$HISTCONTROL` to configure _Bash It_'s automatic history management feature.

theme/codeword: adopt automatic history

ee65fd8

Use `preexec` and `$HISTCONTROL` to configure _Bash It_'s automatic history management feature.

gaelicWizard force-pushed the history_themes branch from 7825eb9 to ee65fd8 Compare February 9, 2022 00:59

gaelicWizard changed the title ~~Update themes to use new Automatic History Management~~ DRAFT: Update themes to use new Automatic History Management Feb 12, 2022

seefood self-assigned this Nov 19, 2024

seefood added seems abandoned rattle the cage, and close if nobody wants to keep it going waiting-for-response labels Nov 19, 2024

Merge branch 'master' into history_themes

26b2a8a

seefood approved these changes May 8, 2025

View reviewed changes

seefood marked this pull request as ready for review May 8, 2025 11:15

akinomyoga suggested changes May 8, 2025

View reviewed changes

seefood closed this May 8, 2025

akinomyoga mentioned this pull request May 8, 2025

Help us clean up Bash-it! #1696

Open

		safe_append_preexec '_bash-it-history-auto-load'
		safe_append_prompt_command '_bash-it-history-auto-save'

	function _bash-it-history-init() {
	safe_append_preexec '_bash-it-history-auto-save'
	safe_append_prompt_command '_bash-it-history-auto-load'
	}

DRAFT: Update themes to use new Automatic History Management #1951

DRAFT: Update themes to use new Automatic History Management #1951

Conversation

gaelicWizard commented Sep 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

gaelicWizard commented Sep 24, 2021

Uh oh!

gaelicWizard commented Feb 20, 2022

Uh oh!

seefood commented Nov 19, 2024

Uh oh!

seefood commented May 8, 2025

Uh oh!

akinomyoga left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

akinomyoga May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seefood May 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

akinomyoga May 8, 2025

Choose a reason for hiding this comment

Uh oh!

akinomyoga May 8, 2025

Choose a reason for hiding this comment

Uh oh!

akinomyoga May 8, 2025

Choose a reason for hiding this comment

Uh oh!

seefood May 8, 2025

Choose a reason for hiding this comment

Uh oh!

akinomyoga May 8, 2025

Choose a reason for hiding this comment

Uh oh!

seefood May 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akinomyoga commented May 8, 2025

Uh oh!

Uh oh!

gaelicWizard commented Sep 18, 2021 •

edited

Loading

akinomyoga left a comment •

edited

Loading

akinomyoga May 8, 2025 •

edited

Loading