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

Update desktop entry file exec keys #3822

Closed
wants to merge 3 commits into from
Closed

Update desktop entry file exec keys #3822

wants to merge 3 commits into from

Conversation

sixg0000d
Copy link
Contributor

@sixg0000d sixg0000d commented Mar 15, 2023

Hello,
I tried to download scrcpy-console.desktop from dev branch and running it, but it seems broken to me. When I run it, it warns "Can't find /usr/bin/bash -i".

So I suggest this update. This update has 2 behavier changes:

  1. Press any key -> Press Enter key
  2. Users has to press the key after scrcpy running correctly and closed as well

As a user I think these are fine. At least the Exec keys in desktop entry files are going to have less backslashes. And they are working fine both on my local bash and fish login shell environments. And they passed desktop entry validation.

I removed the quotes (and the backslahes they need) around $SHELL, I guess the quotes are unnecessary?

I also removed -i option in scrcpy.desktop, because we obviously don't need any interactive shell session in scrcpy.desktop.

@rom1v
Copy link
Collaborator

rom1v commented Mar 15, 2023

Thank you for your PR.

When I run it, it warns "Can't find /usr/bin/bash -i".

It's surprising, since the Exec line contains /bin/bash 🤔 (maybe a wrong copy-paste?)

/bin/sh -i -c "\\$SHELL -i -c scrcpy;echo 'Press Enter to quit...';read"

The message Press Enter to quit and read should only be called when scrcpy fails (to leave time to read the error message), otherwise the console should close.

By replacing read -p msg by echo msg; read, you can use the same read call for many shells, that's good. However, they must be executed only when scrcpy fails, and it's not trivial. For example echo a || ( echo b && echo c ) works in bash and dash, but not fish.
That's why it's easier if we can force a specific shell (bash).

Maybe we should use /usr/bin/env bash … instead? (see 1c44dc2)

Press any key -> Press Enter key

OK

I also removed -i option in scrcpy.desktop

OK

I removed the quotes (and the backslahes they need) around $SHELL, I guess the quotes are unnecessary?

I think they are necessary if the value of $SHELL contains spaces (which is unlikely, but for correctness…)

@rom1v
Copy link
Collaborator

rom1v commented Mar 15, 2023

cc @voyageur (as the author of #3817)

@rom1v
Copy link
Collaborator

rom1v commented Mar 15, 2023

As an alternative to these shell script hacks, I could add an option --wait-on-error which does this in native code (and this would simplify both these .desktop and the Windows version).

@sixg0000d
Copy link
Contributor Author

I revert this commit and create a new one which I doubled backslashes in it and they works for me.

@sixg0000d
Copy link
Contributor Author

sixg0000d commented Mar 15, 2023

As an alternative to these shell script hacks, I could add an option --wait-on-error which does this in native code (and this would simplify both these .desktop and the Windows version).

This sounds nice, too many backslashes and quotes are complexed the exec key command. But seems like we still need quotes and backslashes around $SHELL? I have no idea we can avoid them.

@sixg0000d
Copy link
Contributor Author

sixg0000d commented Mar 15, 2023

It's surprising, since the Exec line contains /bin/bash

it warns:

bash: /bin/bash -i: no such file or directory
Press any key to quit...

It seems equal to when I run bash -i -c "\"/bin/bash -i\"" (I thinks there are some quote escape problems?)

@voyageur
Copy link
Contributor

That's interesting, I can not reproduce that setting my login shell to fish or dash, both gtk-launch (that uses the system .desktop file) or "gio launch" (which you can provide a desktop file to) work fine.
This may be a bug in your desktop environment parser, that said the XDG spec does mention double backslashes so (\") instead of (") should be fine (and works here, and accepted by desktop-file-validate)

Also, +1 on dropping -i from main desktop file, good point

@sixg0000d
Copy link
Contributor Author

That's interesting, I can not reproduce that setting my login shell to fish or dash, both gtk-launch (that uses the system .desktop file) or "gio launch" (which you can provide a desktop file to) work fine. This may be a bug in your desktop environment parser, that said the XDG spec does mention double backslashes so (") instead of (") should be fine (and works here, and accepted by desktop-file-validate)

Also, +1 on dropping -i from main desktop file, good point

You are right, gtk-launch scrcpy-console.desktop works fine on my local too, but kioclient exec scrcpy-console.desktop reproduces the error.

@rom1v
Copy link
Collaborator

rom1v commented Mar 16, 2023

I also removed -i option in scrcpy.desktop, because we obviously don't need any interactive shell session in scrcpy.desktop.

Without -i, it does not work for me.

To understand what happens, changed the command to:

Exec=/bin/sh -c "\"\\$SHELL\" -c 'scrcpy |& tee /tmp/log'"
$ cat /tmp/log
exec: No such file or directory
ERROR: Failed to execute: [adb], [start-server]
ERROR: Could not execute "adb start-server"
ERROR: Could not start adb daemon
ERROR: Server connection failed
scrcpy 2.0 <https://github.com/Genymobile/scrcpy>

So it probably does not read by ADB environment variable from my .bashrc.

@rom1v
Copy link
Collaborator

rom1v commented Mar 16, 2023

As an alternative to these shell script hacks, I could add an option --wait-on-error which does this in native code (and this would simplify both these .desktop and the Windows version).

That's not very good, since scrcpy could fail parsing command line arguments, so before it knows that the flag requesting to pause has been passed. 😕

@rom1v
Copy link
Collaborator

rom1v commented Mar 16, 2023

"Can't find /usr/bin/bash -i".

What is the result of:

echo $SHELL

?

@sixg0000d
Copy link
Contributor Author

sixg0000d commented Mar 16, 2023

I found a way we can dirctly pass system environment variables in Exec, but I don't know if it works just on kde or all of desktop environments.
because I can't find any information about it from the Specification, but it really works.
Just like:

Exec[$e]=$SHELL -c "$ADB"

(In fact, gtk-launch doesn't launch the desktop file from you gaved uri, it launches by application name)

tested, doesn't work on gtk-launch

@sixg0000d
Copy link
Contributor Author

sixg0000d commented Mar 16, 2023

"Can't find /usr/bin/bash -i".

What is the result of:

echo $SHELL

?

Both directly run this command on terminal or run a desktop file which Exec=/bin/sh -i -c "echo $SHELL;read" are returned only /bin/bash

@sixg0000d
Copy link
Contributor Author

I think #3822 (comment) is right, my problem is from KDE desktop entry parser behavieres difference to other desktop environments.

@sixg0000d
Copy link
Contributor Author

So, any changes can't be apply, and I should figure out why KDE behavieres difference to others.

@rom1v
Copy link
Collaborator

rom1v commented Mar 16, 2023

It remains the change to replace the "any key" by "Enter" 😉

@sixg0000d
Copy link
Contributor Author

sixg0000d commented Mar 16, 2023

It remains change to replace the "any key" by "Enter" wink

yeah, I noticed user cannot actually press any key to quit in bash -c 'read -p' command same as sh -c 'read'.

@sixg0000d sixg0000d closed this Mar 16, 2023
@sixg0000d sixg0000d deleted the desktop-entry-files branch March 16, 2023 10:22
rom1v added a commit that referenced this pull request Jun 29, 2023
Add an option to make scrcpy pause just before it returns with an error
(a non-zero exit code). This prevents any host terminal to close so that
error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
rom1v added a commit that referenced this pull request Jun 29, 2023
The terminal opened by scrcpy-console (.bat or .desktop) must not close
if scrcpy terminates with an error, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
rom1v added a commit that referenced this pull request Jun 29, 2023
Add an option to make scrcpy pause just before it returns with an error
(a non-zero exit code). In some cases, this prevents the terminal window
from automatically closing, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
rom1v added a commit that referenced this pull request Jun 29, 2023
The terminal opened by scrcpy-console (.bat or .desktop) must not close
if scrcpy terminates with an error, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
rom1v added a commit that referenced this pull request Jun 30, 2023
Add an option to make scrcpy pause on exit.

Three behaviors are possible:
 - always pause on exit:
    --pause-on-exit
    --pause-on-exit=true
 - never pause on exit:
    (no option)
    --pause-on-exit=false
 - pause when scrcpy returns with an error (a non-zero exit code):
    --pause-on-exit=if-error

This is useful to prevent the terminal window from automatically
closing, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
PR #4130 <#4130>
rom1v added a commit that referenced this pull request Jun 30, 2023
The terminal opened by scrcpy-console (.bat or .desktop) must not close
if scrcpy terminates with an error, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
PR #4130 <#4130>
rom1v added a commit that referenced this pull request Oct 11, 2023
Add an option to make scrcpy pause on exit.

Three behaviors are possible:
 - always pause on exit:
    --pause-on-exit
    --pause-on-exit=true
 - never pause on exit:
    (no option)
    --pause-on-exit=false
 - pause when scrcpy returns with an error (a non-zero exit code):
    --pause-on-exit=if-error

This is useful to prevent the terminal window from automatically
closing, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
PR #4130 <#4130>
rom1v added a commit that referenced this pull request Oct 11, 2023
The terminal opened by scrcpy-console (.bat or .desktop) must not close
if scrcpy terminates with an error, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
PR #4130 <#4130>
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.

3 participants