Could I ask that what is window menu? GMenu library?

I use i3 at before, it treat a momitor as a individual workspace, I think this is a good idea.

(In reply to Isaac Ge from comment #1)
> Could I ask that what is window menu? GMenu library?

No, the window menu is the menu which pops up when you right-click the titlebar. It's implemented in js/ui/windowMenu.js nowadays.


> I use i3 at before, it treat a momitor as a individual workspace, I think
> this is a good idea.

That's completely unrelated to the bug and would be incompatible with the EWMH spec[0] (and thus break clients that rely on it, like 3rd party pagers/docks) ... not to mention massive rewrites of major parts of mutter.

[0] http://standards.freedesktop.org/wm-spec/wm-spec-1.4.html

I still think this would be a nice addition to the window menu

I am working at this feature now.

Created attachment 299816 [details] [review]
Hi anyone I write a simple patch to implement this feature, I think it can work in theory, but all gnome-shell freezed when I click "move to monitor 1" in the menu... So what is wrong? Thank you!

Of course, you should save all work before try this danger patch!

windowMenu: Add option to move different monitor

The window menu has all those workspace related options, but with multiple monitors, it is much more interesting to quickly move a window 'over' to the other monitor.

Might be a nice addition, and should of course only be shown if there are multiple monitors...

https://bugzilla.gnome.org/show_bug.cgi?id=633994

Signed-off-by: acgtyrant <acgtyrant@gmail.com>

Review of attachment 299816 [details] [review]:

gnome-shell does not freeze, the patch is simply wrong and crashes the window manager.

Regarding the UI, I'm not convinced that this is how we want to present the option - for workspace, we use directional entries (Move workspace up/down/left/right), I think this would also be the better option for monitors.

::: js/ui/windowMenu.js
@@ +123,3 @@
+          for (var i = 0; i < nMonitors; i++) {
+            item = this.addAction(_("Move to Monitor " + (i + 1)), Lang.bind(this, function(event) {
+              window.move_to_monitor(i);

Careful with closures - "i" has function scope, so it will always have the value nMonitors at the time the closure is called, which is out of range for a monitor index and hits an assertion.

(In reply to Florian Müllner from comment #6)

> ::: js/ui/windowMenu.js
> @@ +123,3 @@
> +          for (var i = 0; i < nMonitors; i++) {
> +            item = this.addAction(_("Move to Monitor " + (i + 1)),
> Lang.bind(this, function(event) {
> +              window.move_to_monitor(i);
> 
> Careful with closures - "i" has function scope, so it will always have the
> value nMonitors at the time the closure is called, which is out of range for
> a monitor index and hits an assertion.

So I create a closure casually， however it still does not work when I declare variable in that function.

for (var monitorIndex = 0; monitorIndex < nMonitors; monitorIndex++) {
            item = this.addAction(_("Move to Monitor " + monitorIndex), Lang.bind(this, function(event, monitorIndex) {
              window.move_to_monitor(monitorIndex);

 I click the menu item and the log complains that: 

(gnome-shell:16186): Gjs-WARNING **: JS ERROR: Exception in callback for signal: activate: Error: can't convert monitorIndex to an integer
WindowMenu<._buildMenu/item<@resource:///org/gnome/shell/ui/windowMenu.js:125
PopupMenuBase<.addAction/<@resource:///org/gnome/shell/ui/popupMenu.js:467
_emit@resource:///org/gnome/gjs/modules/signals.js:124
PopupBaseMenuItem<.activate@resource:///org/gnome/shell/ui/popupMenu.js:164
wrapper@resource:///org/gnome/gjs/modules/lang.js:169
PopupBaseMenuItem<._onButtonReleaseEvent@resource:///org/gnome/shell/ui/popupMenu.js:125
wrapper@resource:///org/gnome/gjs/modules/lang.js:169

I am confused now, could you advice please...? Thank you!

Of course I know this is not you want to present the option, but I am afraid that I can not go on until I solve this issue.

(In reply to Isaac Ge from comment #7)
> So I create a closure casually， however it still does not work when I
> declare variable in that function.
> 
> for (var monitorIndex = 0; monitorIndex < nMonitors; monitorIndex++) {
>             item = this.addAction(_("Move to Monitor " + monitorIndex),
> Lang.bind(this, function(event, monitorIndex) {
>               window.move_to_monitor(monitorIndex);
> 
>  I click the menu item and the log complains that: 
> 
> (gnome-shell:16186): Gjs-WARNING **: JS ERROR: Exception in callback for
> signal: activate: Error: can't convert monitorIndex to an integer

That's because the callback is called (in js/ui/popupMenu.js) as:

  menuItem.connect('activate', Lang.bind(this, function (menuItem, event) {
            callback(event);                                                    
        }));

In other words: callback is called with a single parameter, thus the second parameter you are using has the value of undefined, which indeed cannot be converted to a number.

What you can do instead is mirroring the index in a local variable each iteration:

for (let i = 0; i < nMonitors; i++) {
    let monitorIndex = i;
    item = this.addAction(_("Move to Monitor " + monitorIndex), Lang.bind(this,
        function() {
            window.move_to_monitor(monitorIndex);
        }));

I am afraid that there is not any mutter relative API that move window between some monitors in a special direction...

There is, it is just not introspected (yet):
https://git.gnome.org/browse/mutter/tree/src/core/screen-private.h#n164

I write out this new code :

let screen = window.get_screen();
let nMonitors = screen.get_n_monitors();
if (nMonitors > 1) {
  this.addMenuItem(new PopupMenu.PopupSeparatorMenuItem());


  for (let monitorIndex = 0; monitorIndex < nMonitors; monitorIndex++) {
    let monitorInfo = screen.get_monitor_neighbor(monitorIndex, Meta.ScreenDirection.UP)
    if (monitorInfo) {
      item = this.addAction(_("Move to Monitor Up"), Lang.bind(this, function(event) {
        window.move_to_monitor(monitorInfo.number);
      }));
    }
  }
}

However it is difficult to map Mutter native API to JavaScript, the log complains after I rebuild and right-click in a titlebar:

(gnome-shell:21618): Gjs-WARNING **: JS ERROR: TypeError: Meta.Screen.Direction is undefined

I tried `Meta.Screen.Direction.UP` too, it is still undefined. Any idea? Thank you!

(In reply to Isaac Ge from comment #11)

>     let monitorInfo = screen.get_monitor_neighbor(monitorIndex,
> Meta.ScreenDirection.UP)

That's the correct way to map MetaScreenDirection, once you expose the type to introspection - did you do that?

(In reply to Florian Müllner from comment #12)

> That's the correct way to map MetaScreenDirection, once you expose the type to introspection - did you do that?

Sorry, but I do not how to "expore the type to introspection"...

Created attachment 300072 [details] [review]
screen: Add public method to get neighboring monitor

The existing private get_monitor_neighbor() function returns a
MetaMonitorInfo, which is private as well. Add a public wrapper
that returns a monitor index instead, as we do for other public
monitor-related methods.

(In reply to Isaac Ge from comment #13)
> Sorry, but I do not how to "expose the type to introspection"...

Yeah, no worries - attached patch should do the job.

Created attachment 300107 [details] [review]
windowMenu: Add option to move different monitor

The window menu has all those workspace related options, but with multiple
monitors, it is much more interesting to quickly move a window 'over' to the
other monitor.

Might be a nice addition, and should of course only be shown if there are
multiple monitors...

Review of attachment 300107 [details] [review]:

Code looks mostly good, thanks!

The commit message needs some cleanup, something like "Might be a nice addition" doesn't really belong there.

::: js/ui/windowMenu.js
@@ +109,3 @@
                 }
                 if (idx < nWorkspaces) {
+                    this.addAction(_("Move to Workspace Down"), Lang.bind(this, function(event) {

Unrelated

@@ +115,3 @@
             }
         }
+        

Trailing whitespace

@@ +117,3 @@
+        
+        let screen = window.get_screen();
+        let nMonitors = screen.get_n_monitors();

Use global.screen

Review of attachment 300107 [details] [review]:

Code looks mostly good, thanks!

The commit message needs some cleanup, something like "Might be a nice addition" doesn't really belong there.

::: js/ui/windowMenu.js
@@ +109,3 @@
                 }
                 if (idx < nWorkspaces) {
+                    this.addAction(_("Move to Workspace Down"), Lang.bind(this, function(event) {

Unrelated

@@ +115,3 @@
             }
         }
+        

Trailing whitespace

@@ +117,3 @@
+        
+        let screen = window.get_screen();
+        let nMonitors = screen.get_n_monitors();

Use global.screen

@@ +143,3 @@
+          let rightMonitorIndex = screen.get_monitor_neighbor_index(monitorIndex, Meta.ScreenDirection.RIGHT);
+          if (rightMonitorIndex != -1) {
+            this.addAction(_("Move to Monitor Down"), Lang.bind(this, function(event) {

This is also obviously wrong :-)

Created attachment 300168 [details] [review]
windowMenu: Add option to move different monitor

The window menu has all those workspace related options, but with multiple monitors, it is much more interesting to quickly move a window 'over' to the other monitor.

Review of attachment 300168 [details] [review]:

The commit message body needs wrapping, otherwise looks good to me now - thanks!

(In reply to Florian Müllner from comment #19)
> Review of attachment 300168 [details] [review] [review]:
> 
> The commit message body needs wrapping, otherwise looks good to me now thanks!

My English is so poor that I am not sure what is the good commit message... The [guide][1] only mentioned that "Says why the change was made (avoid a gcc warning)" However, is this OK?

> windowMenu: Add option to move different monitor
>
> The existing window menu offers all those workspace motion options, add a option to move different monitors as we do for monitor. 

[1]: https://wiki.gnome.org/GnomeLove/SubmittingPatches#A3._Commit

(In reply to Isaac Ge from comment #20)
> (In reply to Florian Müllner from comment #19)
> > Review of attachment 300168 [details] [review] [review] [review]:
> > 
> > The commit message body needs wrapping, otherwise looks good to me now thanks!
> 
> My English is so poor that I am not sure what is the good commit message...
> The [guide][1] only mentioned that "Says why the change was made (avoid a
> gcc warning)" However, is this OK?

The text itself was fine, but commit messages are not auto-wrapped, so you need to include line breaks rather than putting everything in a single line:

The window menu has all those workspace related options, but with multiple
monitors, it is much more interesting to quickly move a window 'over' to the other monitor.

Created attachment 300184 [details] [review]
windowMenu: Add option to move different monitor

The window menu has all those workspace related options, but with multiple
monitors, it is much more interesting to quickly move a window 'over' to the
other monitor.

Comment on attachment 300184 [details] [review]
windowMenu: Add option to move different monitor

Attachment 300184 [details] pushed as 43fc598 - windowMenu: Add option to move different monitor

Attachment 300072 [details] pushed as 2e3086e - screen: Add public method to get neighboring monitor