cointop-sh / cointop

A fast and lightweight interactive terminal based UI application for tracking cryptocurrencies 🚀
https://cointop.sh
Apache License 2.0
3.98k stars 311 forks source link

Cointop overwrites shortcuts config in config.toml #233

Closed lyricnz closed 2 years ago

lyricnz commented 2 years ago

Sometimes it re-adds bindings that were moved:

$ grep last_page config.toml
  "#" = "last_page"

$ ./bin/cointop holdings > /dev/null

$ grep last_page config.toml
  "#" = "last_page"
  "$" = "last_page"

Sometimes it reverts a config change

$ grep toggle_portfolio_balances config.toml
  "z" = "toggle_portfolio_balances"

$ ./bin/cointop holdings > /dev/null

$ grep toggle_portfolio_balances config.toml
  "ctrl+space" = "toggle_portfolio_balances"
lyricnz commented 2 years ago

Perhaps it's a design feature, but many of the default shortcut configurations are not "real" and are ignored from the file. They are bound explicitly in code like keybindings.go

ctrl+s=save NOT FOUND"
ctrl+space=toggle_portfolio_balances NOT FOUND"
scrollup=move_up_or_previous_page NOT FOUND"
>=scroll_right NOT FOUND"
scrolldown=move_down_or_next_page NOT FOUND"
|=toggle_chart_fullscreen NOT FOUND"
ctrl+S=save NOT FOUND"
<=scroll_left NOT FOUND"
E=show_portfolio_edit_menu NOT FOUND"
e=show_portfolio_edit_menu NOT FOUND"
A=toggle_price_alerts NOT FOUND"
\\\\=toggle_table_fullscreen NOT FOUND"
tab=move_down_or_next_page NOT FOUND"
+=show_price_alert_add_menu NOT FOUND"
b=sort_column_balance NOT FOUND"
%=sort_column_percent_holdings NOT FOUND"
lyricnz commented 2 years ago

To solve the first issue, cointop should not add default key bindings for actions that are already defined (ie: if you define "#" = "last_page"; cointop should not add "$" = "last_page")

The second issue is caused by "toggle_portfolio_balances" being missing from the list of actions. Seems this is a bit out of sync with the action-binding function.

lyricnz commented 2 years ago

I suppose we also need to support >1 key that binds to the same action (eg e + E ==> show_portfolio_edit_menu)

lyricnz commented 2 years ago

The PR above:

It's not perfect (around multiple keybindings for the same action), but I think it's better.

miguelmota commented 2 years ago

Thanks for the detailed explanation @lyricnz it's definite a bug. The PR looks great. Also I'd be good with removing the multiple default key options for a single action (e.g. e or E => show_portfolio_edit_menu to be only e => show_portfolio_edit_menu) to keep things simple but no strong opinion here