mmeyer724 / sshmenu

MIT License
56 stars 12 forks source link

Scrollable target list + support for multiple config files #4

Closed feinom closed 8 years ago

feinom commented 8 years ago

If the number of targets exceeded the height of the terminal (number of lines), you couldn't see all the available targets. I have implemented scrolling in the target list to fix this. To get the best experience, you need the shutil.get_terminal_size() function, which was implemented in Python 3.3.

I have also added a simple argument parser to support multiple configuration files, in case there is a need for different menus for servers, switches or other stuff.

mmeyer724 commented 8 years ago

Strangely shutil.get_terminal_size doesn't seem to work on macOS Sierra (Python 3.5.2), however tput lines does work.

Here's the code I used:

def get_terminal_height():
    """Returns the terminal's height"""
    height = os.popen('tput lines', 'r').readline()
    return int(height)

Assuming this also works on Linux, it might be a better choice (portability wise) since we already depend on tput for clearing the screen and moving the cursor.

mmeyer724 commented 8 years ago

I added a few small suggestions, nothing major. After changing out shutil.get_terminal_size for tput lines everything seems to work as expected.

This is definitely a much needed feature, thanks for sending this in.

feinom commented 8 years ago

Thanks! I can make the changes you pointed out tonight. They make sense to me :-)

I wasn't able to get 'tput lines' to report the correct number of lines when loading sshmenu as a module. It would report '24' no matter what the height actually was. We could perhaps use 'tput' on OS X, and 'shutil' on Linux?

mmeyer724 commented 8 years ago

Hmm, which terminal emulator are you using, and what is command you're running to test changes? I use python -m ssshmenu while in the top level directory of the repository for development.

stty size is another option, if a portable solution doesn't exist I'm ok with per-os logic.

Edit: I just found this https://github.com/kennethreitz/clint/blob/master/clint/textui/cols.py#L23

feinom commented 8 years ago

I played around some more with tput, and I managed to figure out the problem. The problem surfaces with this code:

tput = Popen(["tput", "lines"], stdout=PIPE, stderr=PIPE)
height, stderr = tput.communicate()

In the above case, height will always get 24 as it's value.

However, if I remove stderr=PIPE from the code, it returns the correct terminal height. Go figure :-) So this code works for me:

tput = Popen(["tput", "lines"], stdout=PIPE)
height, stderr = tput.communicate()

I'll switch from shutil.get_terminal_size() to using tput now, but if you know why stderr=PIPE causes this behavior, I'd like to know!

Edit: I didn't notice the edit in your last reply. It seems like they have another way of getting width and height, but they're only returning the width. Do you think we should try to use the same method, or should we just stick with the tput way? If we switch, we would have to add some more includes, but it might have some kind of benefit?

feinom commented 8 years ago

By the way, I would also like to add support for the Page Up and Page Down keys, but there seems to be a bug in the readchar library. It looks like they've fixed it, but they haven't made any new releases in a while., so I don't think I'll get it into this pull request. Link to the relevant commit: https://github.com/magmax/python-readchar/commit/7437eb2a4968ecca9a0b07de5a6d6ef165a99e44

mmeyer724 commented 8 years ago

Eh, I'd rather stick with tput, considering sshmenu already requires it.

feinom commented 8 years ago

Yeah, I agree. I think I'm done adding stuff for now, but let me know if you spot anything that can be improved. It's a fun project to work on :-)

mmeyer724 commented 8 years ago

No problem! I'm working on a few modifications myself (invisible cursor while in the menu, fully clearing the screen on resize, and potentially truncating long lines with an ellipsis).

Once that's done I'll release 0.0.4. 👍

feinom commented 8 years ago

Sounds great! Looking forward to testing it :-)