laktak / extrakto

extrakto for tmux - quickly select, copy/insert/complete text without a mouse
MIT License
878 stars 45 forks source link

Allow configuration of popup size and position #53

Closed carlocab closed 3 years ago

carlocab commented 3 years ago

This is a quick-and-dirty implementation of allowing the user to configure the popup size and position.

Popup size is set using @extrakto_popup_size, which takes a comma-separated argument specifying width and height (in that order).

Popup position is set similarly using @extrakto_popup_position for the x and y positions of the popup window, as in the description of display-menu and display-popup in man tmux.

This implementation lacks (and sorely needs) input validation. It will break as soon as the user chooses to provide only one of the width or height. The same goes for the x/y positions.

I'm not a huge fan of the way I use awk to parse the options (do I really need an echo there?), but it should be portable across different versions of awk.

I'll update this pull request when I find time to work out the kinks I outlined above.

carlocab commented 3 years ago

I tried to have a look at how fzf-tmux parses popup options to pass on to tmux, but it's inscrutable bash script. I'll have another look at it if I can't find a good way to improve the code here.

laktak commented 3 years ago

You should replace the awk calls with read (built-in)

IFS=, read popup_width popup_height <<< $popup_size

Otherwise I think it looks good, thanks!

carlocab commented 3 years ago

I think this works well now. I'm not sure how you feel about defining the popup defaults at the top of helpers.sh. I'm happy to change it if you have an alternative preferred approach.

Using get_option to set the defaults is now redundant, since the call to tmux popup in open.sh will catch any missing options. However, I figured it was better to write it this way to keep the code uniform and easy to understand.

I'll look at updating the README to document the added options. Do you want that with this PR, or are you happy with this one to be merged and I'll open another one for the docs?

laktak commented 3 years ago

So you want to have a default in case the user forgets to specify the second value?

How about this:

 -w ${popup_width} -h ${popup_height:-$popup_width}

$popup_width has to contain a value and if $popup_height is missing you just use it again.

I think it's a good idea to change the README in the same PR, thanks!

carlocab commented 3 years ago

So you want to have a default in case the user forgets to specify the second value?

Partly, yes. I want the default in case the user only wants to specify the second value, for example:

set -g @extrakto_popup_size ,50%

This makes a lot of sense for the popup position too. But I suppose if they're already setting the height, they may as well set the width too.

carlocab commented 3 years ago

I have incorporated your suggestions and updated the README. Let me know what you think.

laktak commented 3 years ago

Looks great, thanks!