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

add zenity backend #34

Merged
merged 4 commits into from
Nov 12, 2017
Merged

add zenity backend #34

merged 4 commits into from
Nov 12, 2017

Conversation

wheybags
Copy link
Contributor

Hey, I'd like to use this and don't want to pull in all of gtk as a dependency, so I added a zenity backend, as you suggested in your readme.

@mlabbe
Copy link
Owner

mlabbe commented Sep 17, 2017

This looks promising at a glance. When I have some free time I'll give it a closer look.

Currently, simple_exec.h has a comment saying it was taken from another repo (one of yours). By offering this PR, do you permanently and irrevocably agree to the code being licensed under the same license as the rest of NFD?

@wheybags
Copy link
Contributor Author

wheybags commented Sep 18, 2017 via email

@wheybags
Copy link
Contributor Author

pushed a fix for trailing newlines in zenity output

@mlabbe
Copy link
Owner

mlabbe commented Sep 24, 2017

Keep 'em coming. I have yet to look at these changes (busy life), but I will holistically look at them all as soon as I can.

@wheybags
Copy link
Contributor Author

bump :v (also fixed another stupid issue, surprising what you miss before you try really using something :p)

@mlabbe mlabbe changed the base branch from master to devel November 12, 2017 17:34
@mlabbe mlabbe merged commit 14aacda into mlabbe:devel Nov 12, 2017
@wheybags
Copy link
Contributor Author

Thanks!

@mlabbe
Copy link
Owner

mlabbe commented Nov 12, 2017

Almost there!

I'm noticing the following warnings popping up when building in release_x64 config with gcc 5.4.0:

../../src/simple_exec.h: In function ‘runCommandArray’:
../../src/simple_exec.h:99:13: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result]
write(errPipe[WRITE_FD], &err, 1);
^
../../src/simple_exec.h:131:25: warning: ignoring return value of ‘read’, declared with attribute warn_unused_result [-Wunused-result]
read(errPipe[READ_FD], &errChar, 1);

We should check and correctly handle unexpected byte length reads. Are you up for doing a small PR that fixes these new warnings to the devel branch?

@wheybags
Copy link
Contributor Author

These calls are just a simple 1-char write and read to indicate if the process was successfully execed, not successfully run, it could have exited with nonzero exit status, but this checks whether the call to execv succeeded, ie, does the binary we asked to run exist and is it marked executable. If the exec was successful the process will never get to the write, because it will never return from the exec call.

If the write fails... well shit, ain't much we can do, and the process is about to exit. The read failing is the normal course. We read into errChar, which is initialised to zero. If the exec succeeds, nothing gets written to that pipe, the child runs, exits and the pipe is automatically closed.
In this case, the read returns 0 bytes, and the destination buffer isn't modified, otherwise we write a 1 and that gets read. So, for the read, there's no need to use the return value.

I would happily make a pr to suppress the warnings though?

@mlabbe
Copy link
Owner

mlabbe commented Jan 9, 2021

@wheybags Hello - this is the maintainer of Native FIle Dialogs. The Zenity backend appears to have a discrepancy with the other backends. Can you take a look at #95 and advise how to bring it in line with the other backends? Thanks.

@wheybags
Copy link
Contributor Author

Sorry, having looked at the man page for zenity, I don't think there is any way to do that unfortunately.

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.

2 participants