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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Major improvement in how and when the dashboard is drawn #268

Merged
merged 13 commits into from
Dec 3, 2022

Conversation

yubiuser
Copy link
Member

What does this PR aim to accomplish?:

Major improvement in how and when the dashboard is drawn.

  1. Reacts to terminal resizes and immediately redraws the dashboard
  2. allows to set xoffand yoffas offsets to move the dashboard within the terminal. Fixes Reinstate methods CleanPrintf and CleanEcho? (commit 2b406c1) #266

How does this PR accomplish the above?:

  1. traps WINCH and executes TerminalResize()
  2. uses ANSI escape sequences instead of tput (which has a lot of overhead), making the script more portable
  3. further separates collecting information and displaying them (added GenerateSizeDependendOutput)
  4. adds a CleanExit function to restore the terminal functions on exit

P.S. Adding y-offset was easy, as it was simply moving the "cursor" y-times down. I tried implementing x-offset with left margin functions (DECLRMM, DECSLRM and DECOM). Unfortunately, those are not supported by most terminal emulators (got it working on xterm), see https://terminalguide.namepad.de/mode/ So I needed to implement it on every line of the dasboard individually.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

  • I have read the above and my PR is ready for review. Check this box to confirm

@dschaper
Copy link
Member

Nice!

@yubiuser
Copy link
Member Author

x/y Offset

Screenshot at 2022-10-24 09-54-12


Resizing
(There is still a bit flickering, esp. with the items generated by GenerateSizeDependendOutput())
Peek 2022-10-24 09-55

@yubiuser
Copy link
Member Author

Got rid of the flickering.
Peek 2022-10-24 21-41

Two modifications were necessary:

  1. TerminalResize does not re-draw the dashboard itself, but instead kills the sleep function in NormalPADD(). This will then re-draw the dashboard right away.
  2. Added a small delay before checking the terminal size. This gives the kernel a bit time to report the new size and further reduced "flickering"

@yubiuser yubiuser force-pushed the offset branch 2 times, most recently from a107871 to 7e8a510 Compare October 24, 2022 19:52
@yubiuser yubiuser changed the title [WIP] Major improvement in how and when the dashboard is drawn Major improvement in how and when the dashboard is drawn Oct 24, 2022
@yubiuser yubiuser requested a review from a team October 24, 2022 19:55
@yubiuser yubiuser marked this pull request as ready for review October 24, 2022 19:56
@yubiuser yubiuser mentioned this pull request Oct 24, 2022
@yubiuser yubiuser force-pushed the offset branch 2 times, most recently from db52018 to 6a02535 Compare October 25, 2022 10:17
@DL6ER
Copy link
Member

DL6ER commented Nov 19, 2022

I tested it and it seems to work as expected. How are x and y offset supposed to work? Are they quantities a user has to supply or is it rather meant to automagically center the most suitable PADD variant on any given terminal (may it be as large as it wants).

edit NVM, I found it is a manual process:

setOffsets(){
    # sets x/y-offsets (for shifting the Dashboard within the screen) if CLI arguments were passed
    i=1;
    j=$#;
    while [ $i -le $j ]
    do
        case "$1" in
            "-xoff"  ) xOffset="$2";;
            "-yoff"  ) yOffset="$2";;
        esac
        i=$((i + 1));
        shift 1;
    done
}

but how about the idea of the automatic centering? I could even imagine that this would be a nice by-default option with the possibility to specify ./padd.sh -no-center

@yubiuser
Copy link
Member Author

@rdwebdesign has a follow up PR based on this one that will automatically center.

@rdwebdesign
Copy link
Member

rdwebdesign has a follow up PR based on this one that will automatically center.

I'm just waiting this PR to be approved.

Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, after some further testing, I see another issue when defining offsets. When I specify an Y-offset that is would cause a reduction to the next smaller screen size, the offset is not (or only partially) acknowledged. I guess that's by design. (?)

However, the same does not work for X-offset. Given my terminal is wide enough for a given size, this size will be picked even if I have an X-offset resulting in wrapping to the next line and destroying the entire display.

Pictures

Initial terminal size is 83x30
Screenshot from 2022-11-19 18-48-43
Reduction in Y by 1 (83x29) removes the empty line at the bottom (okay):
Screenshot from 2022-11-19 18-49-25
Further reductions in Y (83x27 in this example) reduces the top offset (okay):
Screenshot from 2022-11-19 18-50-06


(back to initial size of 83x30)
Screenshot from 2022-11-19 18-48-43
Reduction in X by 1 (82x30) causes two additional linebreaks in FTL and SYSTEM causing the Y-offset to decrease:
Screenshot from 2022-11-19 18-51-26
Further reduction in X (81x30) finally causes destruction all over the place:
Screenshot from 2022-11-19 18-52-02

@rdwebdesign
Copy link
Member

I can confirm with a different approach:
starting PADD with a offset bigger than the "margins" will cause problems.

Example:
mega is 80x26.

With a window of 85x30, there are 5 columns more than "mega" needs. If I start PADD with -xoff 6 the lines will break.

The same occurs with -yoff 6, but in this case, the "break" is just rolling the lines up.
The final offset will be smaller, but no visual break.

@rdwebdesign
Copy link
Member

rdwebdesign commented Nov 19, 2022

I think I got it.

At the end of SizeChecker() function, add something like this:

    # Limit the offset to avoid breaks
    xMaxOffset=$(($console_width - $width))
    yMaxOffset=$(($console_height - $height))
    if [ "$xOffset" -gt "$xMaxOffset" ]; then
        xOffset="$xMaxOffset"
    fi
    if [ "$yOffset" -gt "$yMaxOffset" ]; then
        yOffset="$yMaxOffset"
    fi

@DL6ER DL6ER marked this pull request as draft November 19, 2022 20:25
@DL6ER
Copy link
Member

DL6ER commented Nov 19, 2022

Temporarily converting to a draft until we've found a solution. @rdwebdesign's suggestion doesn't solve all cases.

@rdwebdesign
Copy link
Member

My solution only works for my new code (including the center function and all changes I made there).

@rdwebdesign
Copy link
Member

Try this code, please:

SizeChecker(){
    # adding a tiny delay here to to give the kernel a bit time to
    # report new sizes correctly after a terminal resize
    # this reduces "flickering" of GenerateSizeDependendOutput() items
    # after a terminal re-size
    sleep 0.1
    console_height=$(stty size | awk '{ print $1 }')
    console_width=$(stty size | awk '{ print $2 }')

    # Below Pico. Gives you nothing...
    if [ "$console_width" -lt "20" ] || [ "$console_height" -lt "10" ]; then
        # Nothing is this small, sorry
        printf "%b" "${check_box_bad} Error!\n    PADD isn't\n    for ants!\n"
        exit 1
    # Below Nano. Gives you Pico.
    elif [ "$console_width" -lt "24" ] || [ "$console_height" -lt "12" ]; then
        padd_size="pico"
        width=20
        height=10
    # Below Micro, Gives you Nano.
    elif [ "$console_width" -lt "30" ] || [ "$console_height" -lt "16" ]; then
        padd_size="nano"
        width=24
        height=12
    # Below Mini. Gives you Micro.
    elif [ "$console_width" -lt "40" ] || [ "$console_height" -lt "18" ]; then
        padd_size="micro"
        width=30
        height=16
    # Below Tiny. Gives you Mini.
    elif [ "$console_width" -lt "53" ] || [ "$console_height" -lt "20" ]; then
        padd_size="mini"
        width=40
        height=18
    # Below Slim. Gives you Tiny.
    elif [ "$console_width" -lt "60" ] || [ "$console_height" -lt "21" ]; then
        padd_size="tiny"
        width=53
        height=20
    # Below Regular. Gives you Slim.
    elif [ "$console_width" -lt "80" ] || [ "$console_height" -lt "22" ]; then
        padd_size="slim"
        width=60
        height=21
    # Below Mega. Gives you Regular.
    elif [ "$console_width" -lt "80" ] || [ "$console_height" -lt "26" ]; then
        padd_size="regular"
        width=60
        height=22
    # Mega
    else
        padd_size="mega"
        width=80
        height=26
    fi

    # Limit the offset to avoid breaks
    xMaxOffset=$(($console_width - $width))
    yMaxOffset=$(($console_height - $height))

    if [ "$xOffset" -gt "$xMaxOffset" ]; then
        xOffset="$xMaxOffset"
    fi

    if [ "$yOffset" -gt "$yMaxOffset" ]; then
        yOffset="$yMaxOffset"
    fi
}

@DL6ER
Copy link
Member

DL6ER commented Nov 20, 2022

@rdwebdesign Thanks, that surely improves the situation but it overwrites the offsets. Once the terminal got small enough, it won't add the offset if the terminal is made larger again. Surely this isn't an issue if we're dealing with displays with constant resolution, however, it feels strange when we have interactive terminals. This fix will be simple and I'll apply and push it (47d28c2).

However, there is another issue. Assume no offsets are set (to make it easier):

  • 80x26 shows mega (80x26 sized)
  • 80x25 shows regular (60x22 sized)
  • 79x25 shows slim (60x21 sized) although regular would easily fit here as well

My last commit (524ec8f) fixes this.

@DL6ER
Copy link
Member

DL6ER commented Nov 20, 2022

Sorry that this comes piece-by-piece but there is just so much to test on this... I found a typo that broke tiny (fixed in 2735a54) but then wondered why small actually shows less info than tiny:

Screenshot from 2022-11-20 09-26-37
Screenshot from 2022-11-20 09-26-41

There would be enough space in slim to display the exact same version information (slim is both taller and wider than tiny). I'm not changing this right now as it is not a bug but a style decision.

@yubiuser yubiuser marked this pull request as ready for review December 1, 2022 21:51
yubiuser and others added 13 commits December 1, 2022 22:52
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
…l is getting larger (after having been small before)

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Co-authored-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: yubiuser <ckoenig@posteo.de>
@yubiuser
Copy link
Member Author

yubiuser commented Dec 1, 2022

Thanks for your input. Works really nice now. I added rd's suggestion about removing the auxiliary variables and rebased on development.

Ready for re-review

rdwebdesign
rdwebdesign previously approved these changes Dec 1, 2022
@rdwebdesign
Copy link
Member

I approve it too soon...

The mini size is getting broken:
padd_mini

@rdwebdesign rdwebdesign dismissed their stale review December 1, 2022 22:17

mini is broken

@rdwebdesign
Copy link
Member

Note:

I think the issue will be fixed by #272

@yubiuser
Copy link
Member Author

yubiuser commented Dec 2, 2022

Yes. This should be addressed in #272

@yubiuser yubiuser requested a review from rdwebdesign December 2, 2022 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants