Fixed command parsing with -e option

[tsujan]

By using QStringList instead of QString.

Fixes #335 and fixes #665

[tsujan]

The executable command (that comes after -e) should also be parsed, in addition to being considered like a string list. I added a commit for that.

Now, in addition to

qterminal -e APP -option "/path/ha ha"
qterminal -e APP -option '/path/ha ha'

All following combinations also work with QTerminal, as they work with xterm:

qterminal -e 'APP -option "/path/ha ha"'
qterminal -e "APP -option \"/path/ha ha\""
qterminal -e "APP -option '/path/ha ha'"

[yan12125]

Great improvements! Haven't checked details, but I suspect parse_command() may be fragile to future changes. Could you add unit tests? I will find time to setup CI for this repo.

[tsujan]

I added a commit for raw string literals.

Unfortunately, I don't have time to add unit tests here — maybe later and in another PR — but, for example, if FeatherPad is installed, the patch can be tested by creating a text file PATH/ha ha (PATH can also contain spaces) and doing:

qterminal -e fpad -s "PATH/ha ha"
qterminal -e fpad -s 'PATH/ha ha'
qterminal -e fpad -s "PATH/ha"\ ha
qterminal -e fpad -s PATH/ha\ ha
qterminal -e 'fpad -s "PATH/ha ha"'
qterminal -e "fpad -s \"PATH/ha ha\""
qterminal -e "fpad -s 'PATH/ha ha'"
qterminal -e 'fpad -s PATH/ha\ ha'
qterminal -e "fpad -s PATH/ha\ ha"

Of course, the home path can be replaced by ~ only when there is no single/double quote, like in:

qterminal -e fpad -s ~/SUB_PATH/ha\ ha

[tsujan]

I suspect parse_command() may be fragile to future changes.

Strange that I didn't see this important comment!

@yan12125, IMHO, it isn't fragile — because it isn't about the changes in Qterminal — it's either correct or incorrect.

Maybe by "fragile" you meant "dirty". As far as I've seen, parsing codes are usually dirty, because of their complex logic.

[yan12125]

By fragile, I mean if someone else wants to modify that function to support more command patterns, parsing for existing patterns may get broken (regression). Adding tests ensures that won't happen.

I can try to turn current examples into tests this week.

[tsujan]

By fragile, I mean if someone else wants to modify that function to support more command patterns, parsing for existing patterns may get broken

Yes, because of its complex logic. By "complex", I mean a bad thing, which I think is inevitable in this case. A worse example of it can be found in https://github.com/lxqt/libqtxdg/blob/master/src/qtxdg/xdgdesktopfile.cpp.

[yan12125]

A worse example of it can be found in https://github.com/lxqt/libqtxdg/blob/master/src/qtxdg/xdgdesktopfile.cpp.

A good news is that comes with tests ;) https://github.com/lxqt/libqtxdg/blob/master/test/tst_xdgdesktopfile.cpp

[tsujan]

A good news is that comes with tests ;)

I never said I'm against adding tests — why should I? I just can't add them myself, due to lack of time.

[yan12125]

Added tests ;)

By the way, during testing, I noticed that xterm lets the shell to handle complex commands. That might be simpler and more reliable.

$ strace -f -e trace=execve xterm -e 'fpad -s \"PATH/ha ha\"'
execve("/usr/bin/xterm", ["xterm", "-e", "fpad -s \\\"PATH/ha ha\\\""], 0x7ffea7d99e18 /* 102 vars */) = 0
[pid 140230] execve("fpad -s \\\"PATH/ha ha\\\"", ["fpad -s \\\"PATH/ha ha\\\""], 0x556787ea45a0 /* 103 vars */) = -1 ENOENT (沒有此一檔案或目錄)
[pid 140230] execve("/usr/bin/zsh", ["zsh", "-c", "fpad -s \\\"PATH/ha ha\\\""], 0x556787ea45a0 /* 103 vars */) = 0
[pid 140230] execve("/usr/bin/fpad", ["fpad", "-s", "\"PATH/ha", "ha\""], 0x7ffec49129f8 /* 103 vars */) = 0

See: https://github.com/ThomasDickey/xterm-snapshots/blob/master/main.c#L5141

[yan12125]

(Rebased so that tests are run on CI)

[tsujan]

Thanks for the commits!

I noticed that xterm lets the shell to handle complex commands.

Interesting, and worth experimenting.

[tsujan]

As for xterm, I remember that its behavior (not its code) was my criterion. In some complex cases (which I don't remember), Konsole's behavior was different from that of xterm and QTermnal, but they were too artificial.

That being said, counterexamples are very welcome.

[yan12125]

Speaking of complex cases, there is a difference between this pull request and xterm:

Command from the user	Parsed command
xterm -e 'fpad -s \"PATH/ha ha\"'	fpad, -s, "PATH/ha, ha"
qterminal -e 'fpad -s \"PATH/ha ha\"'	fpad, -s, \"PATH/ha, ha\"

Anyway, I think it's OK to have different results for complex cases. It's fine as long as parsing logic is reasonable and consistent.

PS. I use zsh, so xterm calls zsh to parse the command as shown in my earlier comment.

[tsujan]

Speaking of complex cases, there is a difference between this pull request and xterm:

I use bash and the behaviors are exactly the same. I think you've found a problem (or inconsistency) in zsh ;)

[yan12125]

I use bash and the behaviors are exactly the same. I think you've found a problem (or inconsistency) in zsh ;)

Interesting! So parsing arguments within qterminal is better. That avoids differences among shells.

[yan12125]

I think this can be merged now?

[tsujan]

So parsing arguments within qterminal is better.

I think there's no strict standard. Here, I just followed the rules that I considered "obvious". Of course, they could be changed if needed.

I think this can be merged now?

IMHO, the sooner git users have it, the better. The next release will be made soon.

[yan12125]

I think there's no strict standard. Here, I just followed the rules that I considered "obvious". Of course, they could be changed if needed.

Sure!

IMHO, the sooner git users have it, the better. The next release will be made soon.

Oh, good reminder! Let me merge this first and check other PRs.