mmeyer724 / sshmenu

MIT License
56 stars 12 forks source link

Save the last chosen menu entry into config file #15

Closed gitmpr closed 1 year ago

gitmpr commented 1 year ago

Save the last chosen menu entry into config file, read the index upon loading the menu and make it the selected one

mmeyer724 commented 1 year ago

Hi there! It's been a moment since I last worked on this project, but I'm happy to see contributions still rolling through 🙂

This is a great addition, but one thing I would suggest is splitting the config reading/writing out into separate functions, e.g:

def get_last_selected_target_index():
    config = json.loads(resources.user.read(config_name))
    return config.get('saved_last_chosen_index', 0)

def save_last_selected_target_index(index: int):
    config = json.loads(resources.user.read(config_name))
    config['saved_last_chosen_index'] = index
    resources.user.write(config_name, json.dumps(config, indent=4))

We can then call these functions from display_menu, in the same places where you are reading and writing the config currently. This change will help in keeping the size of the display_menu function down, as it's already quite complex.

Please let me know what you think! Thank you 🙏🏻

gitmpr commented 1 year ago

That's a good way to incorporate it in a more readable manageable way. Nice that you consider it so quickly even though the project is somewhat old. I still got a great use out of it. I considered creating something similar with the simple-term-menu library but didn't want to have to write something that probably already existed. I still might though as being able to search through all the entries in the menu would be nice

mmeyer724 commented 1 year ago

That's a good way to incorporate it in a more readable manageable way. Nice that you consider it so quickly even though the project is somewhat old. I still got a great use out of it. I considered creating something similar with the simple-term-menu library but didn't want to have to write something that probably already existed. I still might though as being able to search through all the entries in the menu would be nice

I left two minor nit comments but otherwise this lgtm! Thank you

Btw, if you want searching you should take a look at fzf. You might be able to do something cool with it

gitmpr commented 1 year ago

Alright, great. How to proceed now? Do you want me to make another pull request with the changes you mentioned so that you can merge it with the master branch? Is it not possible on Github to edit my pull request and then merge it?

mmeyer724 commented 1 year ago

Alright, great. How to proceed now? Do you want me to make another pull request with the changes you mentioned so that you can merge it with the master branch? Is it not possible on Github to edit my pull request and then merge it?

I'm actually going to merge it now as-is (given my comments were just nits anyways), and I'll resolve the two small nits in a follow up alongside a version bump

Thank you for the contribution!