Settings changes

More tables would be sensible, and new features should be properly fit into tables (as all so far have been)

I think there’s a midground we can achieve more easily/quickly where we just shuffle the existing settings around a bit, and leave the existing options where they are. Just remove them from the default config, and mark them as deprecated in the docs.

It may also be nice to mirror the crate structure with settings, as this is already grouped in a way that makes more sense. It would also reduce the dependencies a bit.

Longer term, I think KDL would work nicely.

Related:

1 Like

I think that makes sense. Maybe a mapping document (markdown table) would be a good start. After having it confirmed, the code changes should be easier to manage. What do you think? I can give the mapping table a try.

I have never seen or used an application that uses KDL, so I have to look into it. What I can see from the docs, it’s not as legible as TOML or YAML.

A mapping doc sounds great!

1 Like
Old Table Old Setting New Table New Setting
db_path history db_path
key_path client key_path
session_path sync session_path
dialect client dialect
timezone client timezone
update_check client update_check
sync_address sync address
sync_frequency sync frequency
search_mode history.tui search_mode
filter_mode history.tui filter_mode
workspaces history.tui workspaces
filter_mode_shell_up_key_binding history.tui filter_mode_shell_up_key_binding
search_mode_shell_up_key_binding history.tui search_mode_shell_up_key_binding
style history.tui style
inline_height history.tui inline_height
invert history.tui invert
show_preview history.tui.preview enabled
exit_mode history.tui exit_mode
word_jump_mode history.tui word_jump_mode
word_chars history.tui word_chars
scroll_context_lines history.tui scroll_context_lines
ctrl_n_shortcuts history.tui ctrl_n_shortcuts
history_format history command_format
history_filter history command_filter
cwd_filter history directory_filter
max_preview_height history.tui.preview max_height
show_help history.tui show_help
show_tabs history.tui show_tabs
secrets_filter history secrets_filter
enter_accept history.tui enter_accept
keymap_mode history.tui keymap_mode
keymap_cursor history.tui keymap_cursor
network_timeout sync network_timeout
local_timeout client local_timeout
prefers_reduced_motion history.tui reduced_motion
stats common_subcommands history.stats common_subcommands
stats common_prefix history.stats common_prefix
keys scroll_exits history.tui.keys scroll_exits
sync records sync records
preview strategy history.tui.preview strategy
daemon enabled daemon enabled
daemon sync_frequency daemon deleted
daemon socket_path daemon socket_path
daemon tcp_port daemon tcp_port

This is a preliminary table and is supposed to be the starting point.

I have 3 questions/suggestions:

  • it could make sense to use another table for history related settings (I currently put those under client)
  • sync_address and sync_frequency: I’m wondering whether they should be under [sync] as server and frequency and then be used by either the daemon or the client (depending on what is used). afaik they are never used at the same time anyway.
  • update_check: no clue (is this also used when running the server/daemon or only when the client is used?)
1 Like

Agreed, that aligns with dotfiles/etc. I’ll update the table with this

It’s tricky, as there are two. The difficulty is that they have slightly different meaning. A sync_freq of 0 outside the daemon means “sync immediately after the user runs a command”, but in the daemon acts as a timer. So if you set it to 0 there, you’re pretty much just constantly hitting the server.

It might make sense to name them differently. daemon.sync_interval, and sync.frequency, perhaps. The latter is ignored when the daemon is enabled.

It’s actually used by the TUI, but I think makes sense as a more general setting. Perhaps under client

I did not have this specific setting in mind. IMO, db_path is a client setting. Even though it is also used by the daemon, it is primarily a client setting, because it is used on the client. (e.g. if you run a server, this db is never used.)

Those are the settings I thought might be better under a table history:

Old Table Old Setting New Table New Setting
history_format history format
history_filter history command_filter
cwd_filter history directory_filter
secrets_filter history secrets_filter

As a fellow architect/designer/dev I do understand this distinction very well. However, I do not think this distinction should be used in the naming of the settings. It might only confuse users. The specifics can be explained in the documentation. But it all depends whether you move the address and the frequency/interval to the [sync] table.

If you used the address under [sync], and used sync_frequency in [client] and sync_interval in [daemon], it could work. Personally I’d rather move those 2, or better yet only use one (either frequency or interval), under [sync].
Then you can also get rid of the sync_ prefix in the setting name(s).

Currently there is only one setting under [sync]. For me it looks a bit weird to have also sync_xxx settings, even though there’s a sync table.

This is probably very subjective, so never mind in case this sounds stupid to you.

Sounds good to me.

db_path refers to the shell history database. There are actually other sqlite files, such as the one used for records. Without putting it in the history table, it’s pretty ambiguous

Putting record_store_path under client does make sense

I don’t think this logic makes sense. We already have a separate config file for server settings. A history table makes more sense than a client one, really - almost everything I can see under client would work better elsewhere

network_timeout is more of a sync setting, as is session_path

secrets_filter makes sense under history, as do history_format, history_filter, cwd_filter - sounds like we agree there :slight_smile:

local_timeout is pretty ambiguous and would make sense under client

Beyond just a settings change, but I do agree with you that having only one would be nice.

Let’s just go with sync.frequency, and set some sort of minimum in the daemon. We could someday add sync.real_time to enable “sync right after a command” in the daemon too, that people try and achieve with sync_frequency=0

I’ll make some other changes to your table