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

run_scaled: support passing arguments to Xpra #3303

Merged
merged 4 commits into from Oct 19, 2021

Conversation

dancek
Copy link
Contributor

@dancek dancek commented Oct 14, 2021

I needed to pass some arguments to Xpra when running Wine software with run_scaled. I think this might have other use cases than mine, so I added support for passing generic extra arguments.

While I was at it, I fixed a bug in the manpage.

@totaam
Copy link
Collaborator

totaam commented Oct 14, 2021

Thanks!
I think it would be nicer if instead of using --xpra-arg=, the parsing just looked for -- in the arguments and if found then it could just pass on any arguments before it to xpra and those after it to the real command.

@dancek
Copy link
Contributor Author

dancek commented Oct 14, 2021

I actually considered that.

Considering the UI of run_scaled, I'd find it confusing because run_scaled is basically a wrapper around an X application. It also wraps Xpra, but a user won't think of Xpra when they write run_scaled some_application. And since it's conventional to pass arguments after -- to the wrapped software, it would feel like run_scaled some_application -- --param would call some_application --param.

Another point is that running applications that themselves expect the -- separating argument would be difficult to call if run_scaled itself used that.

Now, if --xpra-arg= sounds like a bad idea, I'd consider something like XPRA_ARGS="--arg1 --arg2" run_scaled some_application instead. Is that better?

@dancek
Copy link
Contributor Author

dancek commented Oct 14, 2021

Still, if you do consider the -- separator to be the best approach just say so and I'll implement that 👍

@totaam
Copy link
Collaborator

totaam commented Oct 14, 2021

Another point is that running applications that themselves expect the -- separating argument would be difficult to call if run_scaled itself used that.

In that case, you must specify it before theapp.
run_scaled -- theapp -- --whatever

Still, if you do consider the -- separator to be the best approach just say so and I'll implement that

I do. Sorry for the extra work!
It just feels cleaner. Being a single argument, --xpra-arg= could get messy with nested quoting.
As for XPRA_ARGS=, I've already overused environment variables in xpra!

The run_scaled script supports an argument called --scale, but the
manpage specifies --scaling for some reason. Fix the occurences to match
the implementation.
@dancek
Copy link
Contributor Author

dancek commented Oct 14, 2021

I implemented the -- separator and force-pushed the changed commits. (I think it's cleaner not to have a wrong implementation in git history.)

In that case, you must specify it before theapp. run_scaled -- theapp -- --whatever

This won't work in my implementation. But hopefully no one needs it.

scale = parse_scale(x[len("--scale="):])
elif x == "--":
extra_xpra_args = sys.argv[i+2:]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will pass arguments after -- to xpra and does not take into account he possibility that there might be more than one -- in the command line.
I'll take a stab at it later today.

This allows running commands like

    run_scaled app --args -- --more-args -- --xpra-args

such that the command to be run is "app --args -- --more-args" and the
args passed to Xpra is "--xpra-args"
@dancek
Copy link
Contributor Author

dancek commented Oct 15, 2021 via email

@totaam
Copy link
Collaborator

totaam commented Oct 15, 2021

I would prefer splitting on the first arg, that way you never end up with a command with -- right at the end, which is what you would need to use if there was another -- used by the app.
ie:

run_scaled --scale=1.5 -- someapp -- --someapp-arg

is much nicer than:

run_scaled --scale=1.5 someapp -- --someapp-arg --

Also it keeps the xpra args, including --scale, all on one side. The side that has run_scaled.
So when you do:

run_scaled --dpi=96 --scale=1.5 --title=hello -- someapp --someapp-arg

It's easier to see which arguments go where.
When you split at the end, you have to mentally re-assemble them from either side.

@dancek
Copy link
Contributor Author

dancek commented Oct 15, 2021

That would require everyone to always use --, wouldn't it? Or we could just pass all non-recognized arguments starting with - to Xpra and consider the first argument that starts with something else to be the start of the command. In which case we don't really need -- for anything?

@totaam
Copy link
Collaborator

totaam commented Oct 15, 2021

That would require everyone to always use --, wouldn't it?

No. -- would only be required if either:

  • the application uses it
  • one wants to specify arguments for xpra

Existing users would not need to modify their existing command lines at all, which is important.

@dancek
Copy link
Contributor Author

dancek commented Oct 15, 2021

Right. But why not just allow

run_scaled --dpi=96 --scale=1.5 --title=hello someapp --someapp-arg

where --scale=1.5 is an argument handled by run_scaled, --dpi=96 and --title=hello are passed to Xpra, and the command is someapp --someapp-arg?

Anyway, I'll implement it either way later.

If "--" is present in the arguments, everything after the first "--" is
the command and everything before it is either a run_scaled argument or
(if not recognized) an Xpra argument.
@dancek
Copy link
Contributor Author

dancek commented Oct 18, 2021

run_scaled --dpi=96 --scale=1.5 --title=hello -- someapp --someapp-arg

This works now.

@totaam totaam merged commit 449a2cc into Xpra-org:master Oct 19, 2021
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.

None yet

2 participants