Created attachment 186429 [details] [review]
patch

Easy to fix.  Here's a patch.

Review of attachment 186429 [details] [review]:

No this is wrong, you are changing the semantics of that function instead of just calling the the correct one (i.e in the JS code).

(In reply to comment #0)
> I'm using GNOME Shell 3.0.0.2 on Fedora 15.  I have two monitors: a laptop
> display on the left, and a external monitor on the right.  The external monitor
> is primary: that's where the GNOME Shell panel appears.
> 
> When I choose Log Out from the system menu, if any window is active then GNOME
> Shell displays the Log Out dialog on the monitor containing that window. 
> That's fine.  However, if no window is active the the Log Out dialog appears on
> my laptop display. It should instead appear on the primary (external) monitor.

It appears on the focused monitor (i.e the monitor where you are probably interacting with at the moment).

Changing that is easy the question is whether we want that or not. It would make sense to be consistent with alt-tab and the workspace switcher; but the current situation isn't bad either. So lets gets the designers opinion(s).

Here's how things work today:

- Alt+Tab and the Activities overview always appear on the primary monitor.
- All GNOME Shell dialogs (including the logout dialog, the Alt+F2 Run dialog and authentication dialogs) appear on the focused monitor, i.e. the monitor containing the focused window.  If no window has focus, these dialogs appear on monitor 0 (which might or might not be the primary monitor).

My patch above makes only a tiny change to the above: if no window has focus, we now use the primary monitor rather than monitor 0.  If any window has focus, my patch has no effect.  I think this is clearly a beneficial change.

A larger question is whether some or all dialogs should appear on the primary monitor rather than the focused monitor.  I think that's beyond the scope of this bug report, but you could certainly open another bug if you'd like to propose changing that.

(In reply to comment #4)
> Here's how things work today:
> 
> - Alt+Tab and the Activities overview always appear on the primary monitor.
> - All GNOME Shell dialogs (including the logout dialog, the Alt+F2 Run dialog
> and authentication dialogs) appear on the focused monitor, i.e. the monitor
> containing the focused window.  If no window has focus, these dialogs appear on
> monitor 0 (which might or might not be the primary monitor).
> 
> My patch above makes only a tiny change to the above: if no window has focus,
> we now use the primary monitor rather than monitor 0.  If any window has focus,
> my patch has no effect.  I think this is clearly a beneficial change.

Oh ... sorry misread your patch, yeah that makes sense.

Review of attachment 186429 [details] [review]:

Sorry was wrong before, the patch looks correct.

Remove the "[PATCH]" from the subject and add a commit body, otherwise fine.

Created attachment 186664 [details] [review]
updated patch

Thanks for the review.  Here's an updated patch including a commit body.  I've removed [PATCH] as requested, though I don't actually understand why that's necessary since 'git am' will strip it automatically when applying a commit.

I don't yet have a GNOME account, so can you commit this?

(In reply to comment #7)
> Created an attachment (id=186664) [details] [review]
> updated patch
> 
> Thanks for the review.  Here's an updated patch including a commit body.  I've
> removed [PATCH] as requested, though I don't actually understand why that's
> necessary since 'git am' will strip it automatically when applying a commit.

Yeah it wouldn't do that if you pushed it directly from your tree but given that you don't have a gnome account that does not apply.

> I don't yet have a GNOME account, so can you commit this?

Sure and thanks for the patch.

Bit confused here - I agree that this patch conceptually made sense, but meta_screen_get_primary_monitor always returns 0 at the moment (check mutter/src/screen.c) so I don't see how it changed anything.

Reopening here in case there is some other bug involved, I thought I saw misbehavior once in testing, but couldn't reproduce - in subsequent testing it seemed to work fine with or without this patch.

(In reply to comment #9)
> Bit confused here - I agree that this patch conceptually made sense, but
> meta_screen_get_primary_monitor always returns 0 at the moment (check
> mutter/src/screen.c) so I don't see how it changed anything.

Oh .. I didn't notice that Alex changed shell_global_get_primary_monitor to use meta_screen_get_primary_monitor instead of the GDK code we had before. If this indeed always returns 0 it would break lots of things.

But checking the code in mutter this does not seem to be the case:

int
meta_screen_get_primary_monitor (MetaScreen *screen)
{
  g_return_val_if_fail (META_IS_SCREEN (screen), 0);

  return screen->primary_monitor_index;
}

where the index is set in reload_monitor_infos() (which uses the xinerama information etc etc).

(In reply to comment #10)
> int
> meta_screen_get_primary_monitor (MetaScreen *screen)
> {
>   g_return_val_if_fail (META_IS_SCREEN (screen), 0);
> 
>   return screen->primary_monitor_index;
> }
> 
> where the index is set in reload_monitor_infos() (which uses the xinerama
> information etc etc).

Look what it's set to in reload_monitor_infos().

(In reply to comment #11)
> Look what it's set to in reload_monitor_infos().

Alex claims in http://git.gnome.org/browse/mutter/commit?id=f9b5cdfeb1a that the primary monitor is always the first one returned by Xinerama.

Of course this may now be a case of mutter<->shell incompatibility you mentioned in https://bugzilla.gnome.org/show_bug.cgi?id=609258#c130 ...

What's the status of this bug?

I assume this can be considered fixed. Feel free to reopen with an explanation if it’s not.

*** Bug 670740 has been marked as a duplicate of this bug. ***