xmonad / xmonad-contrib

Contributed modules for xmonad
https://xmonad.org
BSD 3-Clause "New" or "Revised" License
579 stars 271 forks source link

X.P: Add escape hatch for preventing X.P IO #864

Closed dcousens closed 3 months ago

dcousens commented 5 months ago

Rationale

When using XMonad.Prompt for multiple (8+) different use cases in my configuration, I never once needed a command history.

The default of reading and writing a file for the command history opaque to the user is strange to me, and each prompt is sharing and replacing the same prompt-history file... but I digress.

My feature request is to stop that behavior, where historySize = 0 didn't cut the mustard since prompt-history is still reading and writing on every prompt.

~Proposal~

~This pull request adds historyRead and historyWrite configuration options to XPConfig, allowing users to write something like:~

  ...
  , historyRead = \cacheDir -> return empty
  , historyWrite = \history cacheDir -> return ()
  ...

~This could open up functionality for using other state for a command history, but maybe that isn't desired. My main priority with this pull request is to prevent the reading and writing of the prompt-history file, and as such I am marking this as a draft until discussion resolves the best path forward.~

Proposal (current)

historySize = 0 should branch the IO behavior, and prevent any related IO (reading or writing).

Checklist

dcousens commented 5 months ago

Maybe a better path forward is to remove the prompt-history writing behaviour by default in a breaking change, and then supporting users to opt-in by adding this kind of configuration?

  ...
  , historyRead = readPromptHistory "prompt-history"
  , historyWrite = writePromptHistory "prompt-history"
  ...
dcousens commented 5 months ago

@slotThe historyCompletionP needs conf either way, but yes branching as I mentioned is an alternative approach

historySize = 0 could branch the behavior, and prevent any related IO (reading or writing).

I'm happy to implement that if the alternative isn't preferable

dcousens commented 5 months ago

@slotThe I have changed the pull request to pattern match historySize = 0 for readHistory and writeHistory

dcousens commented 5 months ago

@slotThe the only downside to a non-breaking change is that historyCompletionP, if used, will not respect the new behaviour (for better or worse), I think this is the right path for now though

slotThe commented 5 months ago

Both historyCompletion and historyCompletionP aren't very widely used within contrib, so I would even be open for a breaking change there, perhaps making them take an XPConfig (or even just the historySize) as an additional argument. I think the number of (actually useful) custom prompts using these in the wild is relatively low

slotThe commented 5 months ago

@dcousens ping for ^

dcousens commented 5 months ago

@slotThe to clarify, you think I should introduce a breaking change [that you have suggested] in this pull request? Assuming yes, I'll try and do that this weekend

slotThe commented 4 months ago

@dcousens sorry for only getting back to you now: I'm saying that I feel ambivalent about introducing a breaking change for historyCompletion[P]; these functions probably aren't very widely used, and the uses they have inside of contrib are all in the vicinity of an XPConfig argument. However, if you feel otherwise them I'm also fine with merging this as-is

dcousens commented 4 months ago

OK, I'll have a shot at that, I'd prefer that personally

slotThe commented 3 months ago

@dcousens friendly ping :)

dcousens commented 3 months ago

Rebased and added a breaking commit, which ensures that historyCompletion* now accepts an XPConfig

slotThe commented 3 months ago

Thank you!