Code review comment for lp:~ted/ubuntu-app-launch/click-exec

On Wed, 2013-07-24 at 07:22 +0000, Colin Watson wrote:

> + cd ${HOME}/.cache/upstart-app-launch/desktop/${APP_ID}
>
> ${HOME}/.cache/upstart-app-launch/desktop/${APP_ID} is a symlink to the .desktop file, not a directory. You need to get the package directory instead and cd to that. Something like:
>
> CLICK_DIR="$(click pkgdir "${HOME}/.cache/upstart-app-launch/desktop/${APP_ID}")"
> cd "$CLICK_DIR"

That's really confusing. Doesn't that mean that the click packaging
system knows about desktop files then? Why would it be dependent on the
representation of the data? It seems like the packaging system should
only know about the manifest file, so it wouldn't have enough
information to link to a particular file there.

> + CLICK_DIR=`click pkgdir ${APP_ID}`
>
> This looks wrong; click pkgdir doesn't take app IDs (perhaps it should).

Ah, okay. I got confused there. I actually had it with package name in
an earlier revision and I thought you changed it. Undoing that commit
in r70.

> Please quote app IDs for safety.

Done in r71

> +pre-start exec @libexecdir@/desktop-exec ${APP_ID} "${APP_URIS}"
>
> It is odd to pass a list of URIs as a single argument separated by spaces rather than just letting the shell's word-splitting sort it out. Did you really mean to quote ${APP_URIS}? (The C code would need to be adjusted to take URIs as more than one element of argv, but that would seem much more normal anyway.) On the other hand, ${APP_ID} should be quoted.

Yes, I've struggled with how that should be done. Since it is always
passed through the Upstart jobs as a single variable it seemed weird to
have the shell expand it in just that case. For instance, you can't
start a job without making it a single variable. Consistency between
what was passed into the job and what was passed to the exec script
seemed logical.

> My reading of desktop_exec_parse is that it will do the wrong thing when presented with e.g. "%%F".

Yes! Fixed in r72.