Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add none border-lines style #3666

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

afh
Copy link

@afh afh commented Aug 17, 2023

馃毀 This is a work-in-progress follow-up PR on #3650 that attempts to add the none border-lines style.

@nicm: I assume there are off-by-one errors when calculating the width and height of the menu and I may have missed some code that does + 4 for the width or + 2 for the height when figuring out the size and position of the menu.
Maybe the changes in this PR so far help you identify other sections where changes are needed or issues with the code I've written.
I think it might be helpful to get another pair of eyes on this 馃憖

Test using test_menu_border_lines_none.sh
#!/bin/ksh

clear
echo "# Expect double tmux border lines menu when styled using arguments"
echo "% ./tmux menu -x 0 -y 1 -b padded -S bg=orange -T Padded border '' '' -bar '' '' '' baz '' ''"
./tmux menu -x 0 -y 1 -b padded -S bg=orange -T Padded border '' '' -bar '' '' '' baz '' ''

clear
set -A size $(stty size)
awk -vc=脳 -vr=${size[0]} -vl=${size[1]} 'BEGIN{for(i=0;i<r;i++) for(j=0;j<l;j++) printf c}'
echo -e "# Expect no tmux border lines menu with -B even when styled using arguments"
echo "% ./tmux menu -x 0 -y 1 -B -b double -S bg=green -T Menu Title no '' '' -border '' '' '' baz '' ''"
./tmux menu -x 0 -y 1 -B -b double -S bg=green -T Menu Title '' '' -'no border' '' '' '' baz '' ''

clear
set -A size $(stty size)
awk -vc=脳 -vr=${size[0]} -vl=${size[1]} 'BEGIN{for(i=0;i<r;i++) for(j=0;j<l;j++) printf c}'
echo -e "# Expect no tmux title or border lines menu with -B even when styled using arguments"
echo "% ./tmux menu -x 0 -y 1 -B -b double -S bg=green 'No title' '' '' -no '' '' '' border '' ''"
./tmux menu -x 0 -y 1 -B -b double -S bg=green 'No title' '' '' -no '' '' '' border '' ''
Description Screenshot
馃憤 padded border-style image
馃檯 none with title: there seems to be one additional row at the very end of the menu and one additional column on the right image
馃檯 none without title: there seems to be two additional row at the very end of the menu and one additional column on the right image

@afh afh self-assigned this Aug 17, 2023
@nicm
Copy link
Member

nicm commented Aug 17, 2023

  •   bw = ((lines != BOX_LINES_NONE) ? 2 : 2);
    

This is a no-op. Also please just use u_int here not u_short.

  •   u_short two = 2;
    

This is not needed? It should be bw?

@afh
Copy link
Author

afh commented Aug 17, 2023

Oh?! Then this appears to be an older state of the PR, let me investigate.

@nicm
Copy link
Member

nicm commented Aug 17, 2023

You can probably just wrap around by doing the loop like this:

start = s;
do {
     ...
     s = RB_NEXT(...);
     if (s == NULL)
         s = RB_MIN(...);
} while (s != start);

But you will need to walk backwards (RB_PREV/RB_MAX) for previous so you'll need a flag saying whether the loop should go forwards or backwards.

@nicm
Copy link
Member

nicm commented Aug 18, 2023

OK, this kind of seems like it works, but there is one bug, firstly:

  • In menu_prepare, lines is checked before it is updated from menu-border-lines. The options_get_number bit needs to be moved up above the bw assignment.

Then there is some odd stuff...

The menu is too narrow by one character. Changing this from 2 to 3 fixes it, but I'm not sure why, it seems like 2 + bw should be right:

-       screen_init(&md->s, menu->width + 2 + bw, menu->count + 2, 0);
+       screen_init(&md->s, menu->width + 3 + bw, menu->count + bw, 0);

Also the height here seems like it should be + bw not a fixed + 2, but changing it without also changing the width from + 2 to + 3, means the lines wrap...

@nicm
Copy link
Member

nicm commented Aug 18, 2023

Removing MODE_WRAP from the screen stops it wrapping incorrectly when the screen width is + 2 but it is still too narrow...

@afh
Copy link
Author

afh commented Aug 18, 2023

Thanks for your comments, @nicm, much appreciated as always. For today I will focus my attention to other work on my agenda and will get back to this as soon as I can.

@nicm
Copy link
Member

nicm commented Aug 18, 2023

OK I think I have fixed it, it was most of the way there just a couple of minor errors and it was not taking the title into account everywhere. It is complicated because it is easy to make an error in one place but then accidentally correct it in another...

Please try this:
tmux-menu-none.diff.txt

There are three cases which need to be handled:

  1. Borders, with or without title.
  2. No border without title.
  3. No border with title.

So the width is the item width plus two spaces plus optionally two border lines. The height is the item width plus optionally two border lines or a line for the title. Then the item position similarly depends on if there is a border and a title.

So everywhere we need to do this:

if (lines != BOX_LINES_NONE) {
       bw = 2;
       co = 0;
} else {
       bw = 0;
       co = (strlen(menu->title) == 0) ? 0 : 1;
} 

Meaning bw is the width of the border and co is the extra lines for the title.

Then we can say:

width is menu->width + 2 + bw
height is menu->count + bw + co
the first item is at x position bw / 2 + 1 and y position bw / 2 + co

@nicm
Copy link
Member

nicm commented Sep 1, 2023

Does this look/work OK for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants