-
-
Notifications
You must be signed in to change notification settings - Fork 398
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 historycmdcount* configs to limit cmdHistory size #4820
base: master
Are you sure you want to change the base?
Conversation
Add config `historycmdcountalert` and `historycmdcountlimit` to limit `state.cmdHistory` size. If `state.cmdHistory` size is larger then the historycmdcountalert when tridactyl start, tridactyl will reduce its size to the historycmdcountlimit. The cmdHistory size check has a 15 seconds delay, so the start up will not be to slow.
Without config this will be a breaking change.
Im not sure if there are issues about command history size. By the way, I see #4763 so I resolve it in this branch, too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Have you experienced any bugs caused by big command history in practice?
src/lib/config.ts
Outdated
/** | ||
* Reduce the size (line count) of ex command history if the size is larger than the historycmdcountalert when tridactyl started. The size will be reduce to the historycmdcountlimit. | ||
*/ | ||
historycmdcountlimit = 512 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have one setting rather than two. Maybe remove 1/3 when it hits the limit.
512 seems pretty small, I'd set it to something much bigger like 100k. I think most of the time people are searching through it rather than just scrolling up
const alert = await config.getAsync('historycmdcountalert') | ||
const limit = await config.getAsync('historycmdcountlimit') | ||
const cmdHistory = state.cmdHistory | ||
if (cmdHistory.length > alert) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should support -1 for unlimited
@@ -160,7 +160,8 @@ export function getCommandlineFns(cmdline_state: { | |||
// Save non-secret commandlines to the history. | |||
if ( | |||
!browser.extension.inIncognitoContext && | |||
!(func === "winopen" && args[0] === "-private") | |||
!(func === "winopen" && args[0] === "-private") && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should support zero to totally disable saving of history?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I does not like the idea that do State.getAsync('historycmdcountlimit')
everytime before save ex command,
but we already do this while saving history.
I can not remember exactly. My tridactyl is not very fast since I have so many tabs. I believe someone (maybe me?) asked about history command size issue but I can not find it. |
So we do not need two config.
Add config
historycmdcountalert
andhistorycmdcountlimit
to limitstate.cmdHistory
size.If
state.cmdHistory
size is larger then the historycmdcountalert when tridactyl start, tridactyl will reduce its size to the historycmdcountlimit.The cmdHistory size check has a 15 seconds delay,
so the start up will not be to slow.
Feel free to change to a better config name or move the code snippet.