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

console.lua: allow persisting the command history #14681

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guidocella
Copy link
Contributor

Disabled by default.

@guidocella guidocella force-pushed the persist-console-history branch 2 times, most recently from 49d11c5 to 9033060 Compare August 16, 2024 17:39
end

local error
history_file, error = io.open(history_path, 'w')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just append and avoid all the read logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't know whether history commands are new or were read from the history file on startup. Even if we kept track of it that would break history deduplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why history deduplication is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid duplicate lines? We even have an option for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, bit too bloated for my taste, but whatever.

Copy link

github-actions bot commented Aug 18, 2024

Download the artifacts for this pull request:

Windows
macOS

@po5
Copy link
Contributor

po5 commented Aug 20, 2024

An issue with doing a full file write instead of appending is that it will end up overwriting history with a zero-byte file if we're out of disk space. Many existing history scripts suffer from this same issue and shrug it off when they lose your history. Don't want this behavior in a builtin script.
Your approach also does weird reordering of the lines, there's no reason new history from the current instance should be wedged in-between different lines that were already written to the history file.
The way you read history is very naive, you can keep the file open and/or resume reading from the last cursor position, saving you a whole new file read to deduplicate entries. This approach works best with appending to the file, which will fix both the history loss and line reordering.
Would even allow you to cheaply write new entries to the file as they come in while preserving the deduplication step, instead of waiting for a clean shutdown event that may not happen, if you want that. Note you may want to dictate buffering yourself with file:setvbuf("full") and file:flush().

I personally think deduplication on write is overkill, but don't skip over the huge block of text since some stuff still applies.

@guidocella
Copy link
Contributor Author

An issue with doing a full file write instead of appending is that it will end up overwriting history with a zero-byte file if we're out of disk space. Many existing history scripts suffer from this same issue and shrug it off when they lose your history. Don't want this behavior in a builtin script.

That's better than duplicating history lines.

Your approach also does weird reordering of the lines, there's no reason new history from the current instance should be wedged in-between different lines that were already written to the history file.

What? New lines from the current instance are appended to the end.

The way you read history is very naive, you can keep the file open and/or resume reading from the last cursor position, saving you a whole new file read to deduplicate entries. This approach works best with appending to the file, which will fix both the history loss and line reordering.
Would even allow you to cheaply write new entries to the file as they come in while preserving the deduplication step, instead of waiting for a clean shutdown event that may not happen, if you want that. Note you may want to dictate buffering yourself with file:setvbuf("full") and file:flush().

The file is reread to save lines from other mpv instances as written in the comment. Rereading it is not necessary to deduplicate entries or reordering. Saving the last cursor position would break if the beginning of the history file has been modified by other mpv instances.

I personally think deduplication on write is overkill

Deduplication is performed every time a line is added to the history. On write it only deduplicates lines written by other mpv instasnces.

@guidocella guidocella force-pushed the persist-console-history branch from 8b5a133 to 7ee4c39 Compare October 13, 2024 16:56
@guidocella guidocella force-pushed the persist-console-history branch from 7ee4c39 to db59907 Compare October 28, 2024 23:09
@guidocella guidocella force-pushed the persist-console-history branch 2 times, most recently from 8d91fd9 to 9951305 Compare November 27, 2024 14:52
@guidocella guidocella force-pushed the persist-console-history branch from 9951305 to 771c465 Compare December 27, 2024 15:19
@guidocella guidocella force-pushed the persist-console-history branch from 771c465 to 1dd24d6 Compare January 7, 2025 20:43
@guidocella guidocella force-pushed the persist-console-history branch from 1dd24d6 to 5b84f22 Compare January 8, 2025 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants