KittyKatt / screenFetch

Fetches system/theme information in terminal for Linux desktop screenshots.
GNU General Public License v3.0
3.83k stars 453 forks source link

New line causes ascii logo to mess up #606

Open DRSDavidSoft opened 5 years ago

DRSDavidSoft commented 5 years ago

I'm submitting a

[x] bug report
[ ] new distro request

Version 3.8.0

:: Finding distro...found as 'Cygwin '
:: Finding hostname and user...found as 'David@David-PC'
:: Finding kernel version...found as 'i686 CYGWIN_NT-10.0-WOW 2.11.2(0.329/5/3)'
:: Finding current uptime...found as '4d 4h 54m'
:: Finding current package count...found as '678'
:: Finding current shell...found as 'bash 4.4.12'
:: Finding current resolution(s)...found as '3840x2160'
:: Finding desktop environment...found as 'Modern UI/Metro'
:: Finding window manager...found as 'DWM/Explorer'
:: Finding window manager theme...found as '
Themes
Windows Dark.theme
'
:: Finding GTK2 theme...found as 'Not Found'
:: Finding GTK3 theme...found as 'Adwaita'
:: Finding icon theme...found as 'Not Found'
:: Finding user font...found as 'Consolas'
:: Finding current CPU...found as 'Intel Core i5-4460 @ 4x 3.2GHz'
': Finding current GPU...found as 'NVIDIA GeForce GTX 1080
:: Finding current RAM usage...found as '12159MiB / 20418MiB'

Please notice that window manager theme is a multi-line string:

Themes
Windows Dark.theme

this causes the ascii logo to get messed up:

scrf-w10

(in addition to that, notice how ) 64-bit is wrapped up incorrectly)

here's how it should actually look:

wscrf

Thanks for creating this awesome tool!

KittyKatt commented 4 years ago

@DRSDavidSoft Okay, first of all, that 64-bit wrapping issue is really odd. It might be best to start stripping some of the info we're reporting from the OS string to make it more manageable. Not the first case of that string getting really long.

Second of all, I notice that there's no theme at all in your second image. Is that the correct result from inside cygwin? Actually, is this Cygwin or WSL? I don't think we have WSL detection at the moment.

DRSDavidSoft commented 4 years ago

Okay, first of all, that 64-bit wrapping issue is really odd. It might be best to start stripping some of the info we're reporting from the OS string to make it more manageable. Not the first case of that string getting really long.

I actually would suggest breaking it into multiple lines, instead of stripping it!

Either of these will do fine: (Click to show)
David@DAVID-PC
OS: Microsoft Windows 10 Enterprise 64-bit
    (v10.0.17763)
Kernel: ....
or
David@DAVID-PC
OS: Microsoft Windows 10 Enterprise 64-bit
Version: 10.0.17763
Kernel: ....

Second of all, I notice that there's no theme at all in your second image. Is that the correct result from inside cygwin? Actually, is this Cygwin or WSL?

The second screenshot is actually neither Cygwin nor WSL – it's another project written in Powershell. I only used it as a reference here :3

I'm not sure what you mean by "there's no theme at all in your second image." Do you mean the titlebar?

I don't think we have WSL detection at the moment.

It's okay, as I don't intend to use WSL on my machine anytime soon (though I'm sure many others will also appreciate it!)

KittyKatt commented 4 years ago

@DRSDavidSoft I'm referring to this, in screenfetch

image

versus this in the powershell project

image

As you can see, there's no "WM Theme:" or a theme detection of any sort available in the second project. Do you think that pulling a "theme" for screenfetch in this instance is even worth it? Is there even a named theme that it can pull?

I'll need to look into the first part. It may require a special treatment of Cygwin/Windows to put that line there as the output for the ASCII is mostly standardized and the other OSes don't currently need or have a "Version: " line.

DRSDavidSoft commented 4 years ago

@KittyKatt Oh, I see.

there's no "WM Theme:" or a theme detection of any sort available in the second project. Do you think that pulling a "theme" for screenfetch in this instance is even worth it? Is there even a named theme that it can pull?

I believe any kind of information that screnfetch can extract from the OS and show on the screen is useful, including the WM theme!

In this case:

:: Finding window manager theme...found as '
Themes
Windows Dark.theme
'

The Windows Dark.theme part I believe is referring to the Windows theme file that's currently being used, so I guess it'd worth it to show it to the user, especially if the user loads a custom theme file.

I'll need to look into the first part. It may require a special treatment of Cygwin/Windows to put that line there as the output for the ASCII is mostly standardized and the other OSes don't currently need or have a "Version: " line.

Thank you for that! I can provide Windows testing if required, to hopefully solve these small issues :)

liyiheng commented 4 years ago

image

almitydave commented 3 years ago

I solved both these issues locally (Win10/mintty). I commented on the multi-line theme problem on this related issue (caused by output from reg query).

The ") 64-bit" wrapping is because wmic os get version includes \r in the output:

$ wmic os get version | sed 's/\r/<CR>/g; s/ /<space>/g; s/\n/<LF>/g;'
Version<space><space><space><space><space><CR>
10.0.18363<space><space><CR>
<CR>

Line feeds are consumed by the shell, but the carriage return gets into the $distro variable because

distro="$distro (v$(wmic os get version | grep '^10\.' | tr -d ' '))"

leaves the \r at the end, which means the line

mydistro=$(echo -e "$labelcolor OS:$textcolor $distro $sysArch")

has the \r between the $distro and $sysArch variables. I just added \r to the tr -d call:

distro="$distro (v$(wmic os get version | grep '^10\.' | tr -d ' \r'))"

It seems like the other calls to wmic either handle \r or aren't affected by it. In my version (3.9.1), this is line 6245.

DRSDavidSoft commented 3 years ago

I'm afraid this issue still persists, even in 3.9.1:

$ ./screenfetch-dev --version
screenFetch - Version 3.9.1
KittyKatt commented 3 years ago

I need to revisit this soon. Commenting to remind myself.

yanorei32 commented 1 year ago

Hi,

I created patches for the fix for Cygwin, MSYS2 and Git Bash.

You can test the patched version in your environment with the following command.

curl -sL https://github.com/yanorei32/screenFetch/raw/msys-cygwin-patch-combined/screenfetch-dev | bash

TL;DR

Those unexpected behaviours mostly come from line-terminating format compatibility issues between Windows and *NIX utilities. They are fixable with a small patch.

Platform Actual behaviour Expected behaviour
Cygwin
OS and WM Theme are broken.
MSYS2
WM Theme is broken.
Git Bash
Packages†1 and WM Theme are broken.

†1: It's not a line-terminating format issue.

Introduction

Cygwin is mostly UNIX compatible. MSYS2 is just a UNIX-like development environment for Windows development. Git for Bash is like MSYS2, but this environment is developed only for git.

The difference in development motivation between Cygwin, MSYS2 and Git for Bash makes the problem more complicated.

The OS column

The OS column implementation is here.

https://github.com/KittyKatt/screenFetch/blob/f497b8f44de439d1206f04bad18ebdfd8783cd5b/screenfetch-dev#L6364-L6371

Why is Cygwin's OS column is broken?

This code calls WMIC repeatedly. WMIC outputs CRCRLF (0d 0d 0a) style line-terminate †2.

†2: Sorry, I don't know why this output already misconvert to CRCRLF (0d 0d 0a) from CRLF (0d 0a).

$ wmic os get version
Version
10.0.22621

$ wmic os get version | xxd -g 1
00000000: 56 65 72 73 69 6f 6e 20 20 20 20 20 0d 0d 0a 31  Version     ...1
00000010: 30 2e 30 2e 32 32 36 32 31 20 20 0d 0d 0a 0d 0d  0.0.22621  .....
00000020: 0a                                               .
$ wmic os get version | grep '^10\.' | xxd -g 1
00000000: 31 30 2e 30 2e 32 32 36 32 31 20 20 0d 0d 0a     10.0.22621  ...
$ wmic os get version | grep '^10\.' | tr -d ' ' | xxd -g 1
00000000: 31 30 2e 30 2e 32 32 36 32 31 0d 0d 0a           10.0.22621...
$ uname -a
CYGWIN_NT-10.0-22621 DESKTOP-5H6F7L3 3.4.0-341.x86_64 2022-02-13 03:22 UTC x86_64 Cygwin
$ 

Unfortunately, CR are still in the output. It's not good. Your terminal starts carriage return (CR) and shows remaining ")" + $sysArch.

The patch for this problem just added the CR filter | tr -d "\r".

                        elif [[ "$distro" == "Cygwin" || "$distro" == "Msys" ]]; then
                                distro="$(wmic os get caption | sed 's/\r//g; s/[ \t]*$//g; 2!d')"
                                if [[ "$(wmic os get version | grep -o '^10\.')" == "10." ]]; then
-                                       distro="$distro (v$(wmic os get version | grep '^10\.' | tr -d ' '))"
+                                       distro="$distro (v$(wmic os get version | tr -d "\r" | grep '^10\.' | tr -d ' '))"
                                fi
                                sysArch=$(wmic os get OSArchitecture | sed 's/\r//g; s/[ \t]*$//g; 2!d')
                                mydistro=$(echo -e "$labelcolor OS:$textcolor $distro $sysArch")

Wait, Why the MSYSs OS column shows expected?

Because, MSYS grep converts implicity any CRLF (0d0a) and CRs+LF (many 0d + 0a) to LF (0a).

In MSYS (CR is filtered by grep):

$ echo -en "\r\n" | xxd -g1
00000000: 0d 0a                                     ..
$ echo -en "\r\n" | grep "" | xxd
00000000: 0a                                       .
$ echo -en "\r\r\n" | grep "" | xxd
00000000: 0a                                       .
$ echo -en "\r\r\r\n" | grep "" | xxd
00000000: 0a                                       .
$ 

In Cygwin and more standard Linux distributions (CR is not filtered by grep):

$ echo -en "\r\n" | xxd -g1
00000000: 0d 0a                                            ..
$ echo -en "\r\n" | grep "" | xxd -g1
00000000: 0d 0a                                            ..
$ echo -en "\r\r\n" | grep "" | xxd -g1
00000000: 0d 0d 0a                                         ...
$ echo -en "\r\r\r\n" | grep "" | xxd -g1
00000000: 0d 0d 0d 0a                                      ....
$

I think the behaviour makes comfortable the development with the Windows toolchains. Is the patch compatible? Yes, that is the just filter of CR (0d), and in this case, that takes no effect.

The WM Theme column

The WM Theme column implementation is here.

https://github.com/KittyKatt/screenFetch/blob/f497b8f44de439d1206f04bad18ebdfd8783cd5b/screenfetch-dev#L2468-L2485

Shows file extensions .theme unexpectedly only in Cygwin.

This issue is very similar to the OS column issue. MSYS sed converts CRLF to LF too. This problem can fix with insertion | tr -d "\r" to two lines.

 elif [[ "${distro}" == "Cygwin" || "${distro}" == "Msys" ]]; then 
    if [ "${WM}" == "Blackbox" ]; then 
        if [ "${distro}" == "Msys" ]; then 
            Blackbox_loc=$(reg query 'HKLM\Software\Microsoft\Windows NT\CurrentVersion\WinLogon' //v 'Shell') 
        else 
            Blackbox_loc=$(reg query 'HKLM\Software\Microsoft\Windows NT\CurrentVersion\WinLogon' /v 'Shell') 
        fi 
-       Blackbox_loc="$(echo "${Blackbox_loc}" | sed 's/.*REG_SZ//' | sed -e 's/^[ \t]*//' | sed 's/.\{4\}$//')" 
+       Blackbox_loc="$(echo "${Blackbox_loc}" | tr -d "\r" | sed 's/.*REG_SZ//' | sed -e 's/^[ \t]*//' | sed 's/.\{4\}$//')" 
        Win_theme=$(grep 'session.styleFile' "${Blackbox_loc}.rc" | sed 's/ //g' | sed 's/session\.styleFile://g' | sed 's/.*\\//g') 
    else 
        if [[ "${distro}" == "Msys" ]]; then 
            themeFile="$(reg query 'HKCU\Software\Microsoft\Windows\CurrentVersion\Themes' //v 'CurrentTheme')" 
        else 
            themeFile="$(reg query 'HKCU\Software\Microsoft\Windows\CurrentVersion\Themes' /v 'CurrentTheme')" 
        fi 
-       Win_theme=$(echo "$themeFile" | awk -F"\\" '{print $NF}' | sed 's|\.theme$||') 
+       Win_theme=$(echo "$themeFile" | tr -d "\r" | awk -F"\\" '{print $NF}' | sed 's|\.theme$||') 
    fi 
 else

Shows multi lines unexpectedly

Its simple bug can fix with | grep CurrentTheme.

        if [[ "${distro}" == "Msys" ]]; then 
            themeFile="$(reg query 'HKCU\Software\Microsoft\Windows\CurrentVersion\Themes' //v 'CurrentTheme')" 
        else 
            themeFile="$(reg query 'HKCU\Software\Microsoft\Windows\CurrentVersion\Themes' /v 'CurrentTheme')" 
        fi 
-       Win_theme=$(echo "$themeFile" | tr -d "\r" | awk -F"\\" '{print $NF}' | sed 's|\.theme$||') 
+       Win_theme=$(echo "$themeFile" | tr -d "\r" | grep CurrentTheme | awk -F"\\" '{print $NF}' | sed 's|\.theme$||') 
$ reg query 'HKCU\Software\Microsoft\Windows\CurrentVersion\Themes' /v 'CurrentTheme'
HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Themes
    CurrentTheme    REG_SZ    C:\Windows\resources\Themes\dark.theme
$ reg query 'HKCU\Software\Microsoft\Windows\CurrentVersion\Themes' /v 'CurrentTheme' | grep CurrentTheme
    CurrentTheme    REG_SZ    C:\Windows\resources\Themes\dark.theme
$

Packages column

The Packages column implementation is here.

https://github.com/KittyKatt/screenFetch/blob/f497b8f44de439d1206f04bad18ebdfd8783cd5b/screenfetch-dev#L1463-L1477

pacman command not found in Git for Windows

KittyKatt/screenfetch detects Git for Windows as MSYS.

The packages routine will go to Msys if-branch, but Git for Windows does not have any package manager. (Because this environment only for git.)

                'Msys')
-                       pkgs=$(pacman -Qq | wc -l)
-                       if [ -d "/c/ProgramData/chocolatey/lib" ]; then
-                               chocopkgs=$(ls -1 /c/ProgramData/chocolatey/lib | wc -l)
-                               pkgs=$((pkgs + chocopkgs))
+                       # Git for Windows (Msys-based) does not have any package manager
+                       if command -v pacman &> /dev/null; then
+                               # for Msys
+                               pkgs=$(pacman -Qq | wc -l)
+                               if [ -d "/c/ProgramData/chocolatey/lib" ]; then
+                                       chocopkgs=$(ls -1 /c/ProgramData/chocolatey/lib | wc -l)
+                                       pkgs=$((pkgs + chocopkgs))
+                               fi
+                       else
+                               # for Git for Windows
+                               if [ -d "/c/ProgramData/chocolatey/lib" ]; then
+                                       chocopkgs=$(ls -1 /c/ProgramData/chocolatey/lib | wc -l)
+                                       pkgs="$pkgs + $chocopkgs" # shows "Unknown + 1234"
+                               fi
                        fi                         

Lastly...

I'll send two pull-request one for CRLF and simple bugfix and one for Git for windows pacman related bugfix.

I hope these problems are resolved early.

Thanks,

nPHYN1T3 commented 3 months ago

Seeing this here with Resolution

       ##O#O##              Shell: bash 5.2.26
        #######              Resolution: 5760x1080
5760x1080
      ###########            WM: ctwm