leoheck / kiri

Kiri is a visual tool designed for reviewing schematics and layouts of KiCad projects that are version-controlled with Git.
MIT License
493 stars 35 forks source link

Fix inverted logic for checking if DISPLAY set #73

Closed MarcelRobitaille closed 1 year ago

MarcelRobitaille commented 1 year ago

I think -n and -z were mixed up here. The previous code overwrote the DISPLAY variable if it's already set. This update only sets a default value if DISPLAY is NOT already set.

From man test:

       -n STRING
              the length of STRING is nonzero
       -z STRING
              the length of STRING is zero
leoheck commented 1 year ago

Hi, thanks for finding this. Do you use a different DISPLAY than the ":1.0" on your system? I may have used the wrong test since the default option was already in use by my OS.

I added some echos and an exit to check this behavior, closely.

Using the proposed -z flag it works this way. image

The -z works when the DISPLAY is already set, and it does not work when the DISPLAY is empty. I saw the DISPLAY empty mostly on WLS on Windows.

Using -n it works this way: image

But, using the -n, the already set DISPLAY is overridden as you showed me. However, it works nicely when DISPLAY is not set.

So, I am seeing both solutions do not work completely. We have to use something else.

After some tests, I could find a way to catch both scenarios. image

Do you want to update your PR? Test with this test if it works for you, and if it works, I will merge it.

if [ -z "" -a -z ${DISPLAY+x} ]; then

If you don't mind I will fix it myself.

MarcelRobitaille commented 1 year ago

On all of my systems, DISPLAY=:0.

When I replaced echo -e "Current DISPLAY = ${DISPLAY:=[EMPTY]}" with echo -e "Current DISPLAY = ${DISPLAY}", everything worked perfectly for me with -z.

My test script:

#!/bin/bash

echo -e "\n==========================================="
echo -e "Current DISPLAY = ${DISPLAY}"
if [[ -z ${DISPLAY} ]]; then
    export DISPLAY=:1.0
    echo -e "Setting DISPLAY = ${DISPLAY}"
fi
echo -e "===========================================\n"

My results:

~ via  v3.12.0a5
❯ /tmp/test_display.sh

===========================================
Current DISPLAY = :0
===========================================

~ via  v3.12.0a5
❯ DISPLAY= /tmp/test_display.sh

===========================================
Current DISPLAY =
Setting DISPLAY = :1.0
===========================================

~ via  v3.12.0a5
❯ unset DISPLAY

~ via  v3.12.0a5
❯ /tmp/test_display.sh

===========================================
Current DISPLAY =
Setting DISPLAY = :1.0
===========================================

Is [EMPTY] really a thing in bash? After the line echo -e "Current DISPLAY = ${DISPLAY:=[EMPTY]}", I tried doing echo ${#DISPLAY} which printed 7. I think this line is setting DISPLAY to "[EMPTY]". Since this is not an empty string, the code isn't going into the -z branch.

leoheck commented 1 year ago

Can you test this, if [ -z "" -a -z ${DISPLAY+x} ]; then instead of if [[ -z ${DISPLAY} ]]; then ?