diff mbox

[V2,2/3] Cocoa: avoid displaying window when command-line contains '-h' or '-help'

Message ID 1306707770-14632-3-git-send-email-cerbere@gmail.com
State New
Headers show

Commit Message

Alexandre Raymond May 29, 2011, 10:22 p.m. UTC
There was already a check in place to avoid displaying a window
in certain modes such as vnc, nographic or curses.

Add a check for '-h' and '-help' to avoid displaying a window for a split-
second before showing the usage information.

Signed-off-by: Alexandre Raymond <cerbere@gmail.com>
---
 ui/cocoa.m |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Comments

Peter Maydell May 29, 2011, 10:32 p.m. UTC | #1
On 29 May 2011 23:22, Alexandre Raymond <cerbere@gmail.com> wrote:
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 1ff1ac6..e1312d3 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -872,7 +872,8 @@ int main (int argc, const char * argv[]) {
>             if (opt[1] == '-') {
>                 opt++;
>             }
> -            if (!strcmp(opt, "-vnc") ||
> +            if (!strcmp(opt, "-h") || !strcmp(opt, "-help") ||
> +                !strcmp(opt, "-vnc") ||
>                 !strcmp(opt, "-nographic") ||
>                 !strcmp(opt, "-version") ||
>                 !strcmp(opt, "-curses")) {

(1) presumably this doesn't work if you disable the display
with "-display none" ?
(2) it's pretty ugly and not very maintainable -- is there
some restructuring possible to avoid having to hardcode
information about qemu options into the ui code here?

(It also doesn't catch other cases where qemu prints some
information and exits immediately, like "-cpu ?".)

-- PMM
Alexandre Raymond May 29, 2011, 10:40 p.m. UTC | #2
I agree that this is not the best way to handle all cases not requiring a GUI.

However, due to the current structure of the code, it was a simple way
to cover a very common case without having to refactor the whole cocoa
code.

Alexandre

On Sun, May 29, 2011 at 6:32 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 29 May 2011 23:22, Alexandre Raymond <cerbere@gmail.com> wrote:
>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>> index 1ff1ac6..e1312d3 100644
>> --- a/ui/cocoa.m
>> +++ b/ui/cocoa.m
>> @@ -872,7 +872,8 @@ int main (int argc, const char * argv[]) {
>>             if (opt[1] == '-') {
>>                 opt++;
>>             }
>> -            if (!strcmp(opt, "-vnc") ||
>> +            if (!strcmp(opt, "-h") || !strcmp(opt, "-help") ||
>> +                !strcmp(opt, "-vnc") ||
>>                 !strcmp(opt, "-nographic") ||
>>                 !strcmp(opt, "-version") ||
>>                 !strcmp(opt, "-curses")) {
>
> (1) presumably this doesn't work if you disable the display
> with "-display none" ?
> (2) it's pretty ugly and not very maintainable -- is there
> some restructuring possible to avoid having to hardcode
> information about qemu options into the ui code here?
>
> (It also doesn't catch other cases where qemu prints some
> information and exits immediately, like "-cpu ?".)
>
> -- PMM
>
Andreas Färber June 1, 2011, 10:16 p.m. UTC | #3
Am 30.05.2011 um 00:22 schrieb Alexandre Raymond:

> There was already a check in place to avoid displaying a window
> in certain modes such as vnc, nographic or curses.
>
> Add a check for '-h' and '-help' to avoid displaying a window for a  
> split-
> second before showing the usage information.
>
> Signed-off-by: Alexandre Raymond <cerbere@gmail.com>

Thanks, applied to the cocoa branch.

> ---
> ui/cocoa.m |    3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 1ff1ac6..e1312d3 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -872,7 +872,8 @@ int main (int argc, const char * argv[]) {
>             if (opt[1] == '-') {
>                 opt++;
>             }
> -            if (!strcmp(opt, "-vnc") ||
> +            if (!strcmp(opt, "-h") || !strcmp(opt, "-help") ||
> +                !strcmp(opt, "-vnc") ||
>                 !strcmp(opt, "-nographic") ||
>                 !strcmp(opt, "-version") ||
>                 !strcmp(opt, "-curses")) {
> -- 
> 1.7.5
Andreas Färber June 1, 2011, 11:05 p.m. UTC | #4
Am 30.05.2011 um 00:32 schrieb Peter Maydell:

> On 29 May 2011 23:22, Alexandre Raymond <cerbere@gmail.com> wrote:
>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>> index 1ff1ac6..e1312d3 100644
>> --- a/ui/cocoa.m
>> +++ b/ui/cocoa.m
>> @@ -872,7 +872,8 @@ int main (int argc, const char * argv[]) {
>>             if (opt[1] == '-') {
>>                 opt++;
>>             }
>> -            if (!strcmp(opt, "-vnc") ||
>> +            if (!strcmp(opt, "-h") || !strcmp(opt, "-help") ||
>> +                !strcmp(opt, "-vnc") ||
>>                 !strcmp(opt, "-nographic") ||
>>                 !strcmp(opt, "-version") ||
>>                 !strcmp(opt, "-curses")) {
>
> (1) presumably this doesn't work if you disable the display
> with "-display none" ?

I don't see how that would not work. It's just not handled specially  
here, so it will likely display a window - the former behavior of all  
these switches.

> (2) it's pretty ugly and not very maintainable -- is there
> some restructuring possible to avoid having to hardcode
> information about qemu options into the ui code here?
>
> (It also doesn't catch other cases where qemu prints some
> information and exits immediately, like "-cpu ?".)

My saying! It's a general problem though: On my GNOME desktop I have  
some launchers for frequently used QEMU machines; it did occur that  
something changed in QEMU and nothing at all happened when double- 
clicking and I had to repeat the same in a terminal to find out why.  
Similar back when using a bundled Q.app on Mac OS X (i.e., a process  
that does not display a Terminal window).

What I have asked for in the past is an override mechanism for error  
messages, so that at runtime we can detect properly whether we're  
running in console or window mode and choose to display a MessageBox  
on Windows, a modal sheet on Mac OS X, a BAlert or whatever a frontend  
author sees fit. Sequential fprintf(stderr, ...) is not really helpful  
for that use case.

The added difficulty for Cocoa is that it needs to go through  
Objective-C (e.g., ui/cocoa.m).

Since that is a larger task and a long-time open issue, I see no  
reason not to accept this patch as an interim solution.

Andreas


P.S. I haven't found any VNC viewer component either, to resort to a  
specialized virt-manager-like graphical interface process with child  
QEMU processes. So going down the VNC route as once under discussion  
would mean forking and maintaining a VNC client for a particular less- 
common platform, which I am not comfortable with, given the occasional  
protocol extensions contributed to QEMU.
Peter Maydell June 2, 2011, 6:10 a.m. UTC | #5
On 2 June 2011 00:05, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 30.05.2011 um 00:32 schrieb Peter Maydell:
>> (1) presumably this doesn't work if you disable the display
>> with "-display none" ?
>
> I don't see how that would not work. It's just not handled specially here,
> so it will likely display a window - the former behavior of all these
> switches.

Yes, that's my point -- this chunk of code is trying to implement
"don't bring up the display window if we won't use it", and it
doesn't work properly [ie does not suppress the window in all the
cases where it should] because inevitably it lags behind the
set of options in vl.c which don't bring up the display window.

I don't particularly object to the patch being committed as a short
term measure.

-- PMM
diff mbox

Patch

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 1ff1ac6..e1312d3 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -872,7 +872,8 @@  int main (int argc, const char * argv[]) {
             if (opt[1] == '-') {
                 opt++;
             }
-            if (!strcmp(opt, "-vnc") ||
+            if (!strcmp(opt, "-h") || !strcmp(opt, "-help") ||
+                !strcmp(opt, "-vnc") ||
                 !strcmp(opt, "-nographic") ||
                 !strcmp(opt, "-version") ||
                 !strcmp(opt, "-curses")) {