Fix drag & drop of menus

[gm10]

Wrong parameter type was getting passed.

[raveit65]

@yetist Do we need this for 1.22 release?

[lukefromdc]

I cannot "move up" or "move down" a menu item at all in the main menu EDIT: so how do I test this?

[gm10]

Yeah, I just made an additional change, the .dom was wrong, too. The change now prevents a crash but nothing actually gets moved. I'll leave that last bit to you guys. Just wanted to prevent the menu going kaputt.

[lukefromdc]

It is with master that I cannot move anything but nothing crashes

[gm10]

@lukefromdc does it not? Due to a string instead of a list getting passed you should end up with something like this in the DOM when trying to move a menu, which then effectively disables both the editor and the menu itself:

    <Menu>
        <Name>S</Name>
        <Menu>
            <Name>y</Name>
            <Menu>
                <Name>s</Name>
                <Menu>
                    <Name>t</Name>
                    <Menu>
                        <Name>e</Name>
                        <Menu>
                            <Move>
                                <Old/>
                                <New>System</New>
                            </Move>
                            <Name>m</Name>
                        </Menu>
                    </Menu>
                </Menu>
            </Menu>
        </Menu>
    </Menu>

[lukefromdc]

If I run mozo (master) from terminal EDIT(and try to move something) I get this:

Traceback (most recent call last):
  File "/usr/lib/python3.7/dist-packages/Mozo/MainWindow.py", line 596, in on_move_up_button_clicked
    self.editor.moveItem(item, item.get_parent(), before=before)
  File "/usr/lib/python3.7/dist-packages/Mozo/MenuEditor.py", line 756, in moveItem
    self.__positionItem(parent, item, before=before, after=after)
  File "/usr/lib/python3.7/dist-packages/Mozo/MenuEditor.py", line 760, in __positionItem
    contents = self.getContents(parent)
  File "/usr/lib/python3.7/dist-packages/Mozo/MenuEditor.py", line 243, in getContents
    item_iter = item.iter()
AttributeError: 'TreeEntry' object has no attribute 'iter'

but the mozo GUI remains responsive, and if I go back to the main menu the menuitem is still there and still works. I have not noticed any corruption of the test menuitem, but I might NOT know what to look for and it is still operable, still opening the program called when clicked from the menubutton.

[gm10]

Oh, so that's a separate bug I hadn't noticed. The DOM output above is from an 1.20 version but that bug also still exists in master and you'll encounter it once you fix the one you're seeing right now, so you'll want to fix it either way (or redo the whole thing since it apparently never worked anyway.

Here's the relevant part in master expecting the list, see the change in the PR for where it was receiving a string instead: https://github.com/mate-desktop/mozo/blob/master/Mozo/MenuEditor.py#L581

[yetist]

Yes, I confirmed this bug, there is no problem to move menu directory, but there is a problem with moving menu entry. I am researching and there will be patches soon.

[yetist]

@gm10

Please test #49, is that ok?

[gm10]

@yetist apologies, it seems we have been talking about different functions here, I should have been more specific. My problem is with MainWindow.py's on_menu_tree_drag_data_received calling MenuEditor.py's moveMenu. This allows drag & drop of menu entries (including root categories, where the up/down buttons you were patching for items are disabled so I hadn't even paid attention to them) and leads to the problem I was trying to fix here.

This was just a spot fix after a Mint user reported killing off their menu with the editor, personally I hadn't used the editor before so sorry for having tunnel-vision in my problem description. ;)

[lukefromdc]

I just tested https://github.com/mate-desktop/mozo/pull/49 and it works, so long as you are using the "move up" and "move down" menuitems rather than trying to drag and drop them

[lukefromdc]

Can this be closed now that https://github.com/mate-desktop/mozo/pull/49 is merged?

[gm10]

@lukefromdc see my last post above, #49 had nothing to do with this.

[lukefromdc]

OK, is there any way I can test this with https://github.com/mate-desktop/mozo/pull/49 merged (and thus applied) given I have not been able to duplicate the menu corruption? My build is against python 3.7, and the Mint user reporting the original issue would have been on MATE 1.20 or earlier and thus using a python 2.7 build

[gm10]

If you're not getting the corruption when dragging and dropping menus/categories then you can probably ignore this, although it would surprise me because the code in master is still passing a string type (https://github.com/mate-desktop/mozo/blob/master/Mozo/MenuEditor.py#L429) to a function expecting an array/list type (https://github.com/mate-desktop/mozo/blob/master/Mozo/MenuEditor.py#L581).

[lukefromdc]

OK, thanks. I will leave this for testing by those who can duplicate the issue

[yetist]

@gm10

You can try #50 , maybe that PR fixed your issues.

[gm10]

@yetist, your __getXmlMenu() change is good and prevents the corruption, but the rest didn't work at least when applied to 1.20.

Looking at what you were trying to do it became apparent what's wrong though, so I added a second commit here that addresses that. Maybe you could review that instead (the situation with the two parallel PRs for the same thing is a bit weird -- if you prefer to keep it in-house I won't mind).

[yetist]

@gm10

49 is also needed on 1.20, so you can just cherry-pick ae480e9fb7237e6c2d0608d4491ba4a6617c64f9 to 1.20 for test.

[yetist]

@gm10

Maybe there are some things I didn't fully understand. I suggest you refer to #47, #49, #50, and do some more tests. If you can modify it better, I don't mind using your PR, :). We welcome more people to contribute code.

[gm10]

@yetist it's not that easy, unfortunately, with both #49 and #50 applied to 1.20 it crashes both the editor and the menu when dragging & dropping a category.

** (mozo:12597): WARNING **: 09:25:43.624: To add siblings to a menu node, it must not be the root node, and must be linked in below some root node
node parent = (nil) and type = 9
**
ERROR:matemenu-tree.c:2524:move_children: assertion failed: (menu_layout_node_get_next (from_child) == insert_before)
Aborted

49 wasn't a problem on 1.20, anyway.

I don't know if you guys actually still support 1.20 so I'm not sure if it matters to you, but if you do feel free to cherry-pick my changes for 1.20 and if yours work on 1.21+ then maybe go with yours going forward? Or we could just fork the thing for LM19.x only and go back to yours for LM20.

If you go with yours do test if you can drag & drop categories as well as custom menus into other categories/menu and also out of them again into root though - with your #50 standalone I could move them deeper into the hierarchy but not higher, that's why I realized the source path was wrong (it wasn't passing the full path).

[yetist]

I remembered that #49 could not be applied to mozo 1.20, we changed the api in mate-menus 1.21. Mozo 1.22 uses the new api, but mozo 1.20 uses the old api, so #49 and #50 may be don't work for mozo 1.20.

The question now is, do we still need to continue to fix the mozo 1.20 bug? @mate-desktop/core-team

[yetist]

If there is the same changes can fix mozo 1.20 and 1.22 is the best, is that #47 good on mozo 1.20 and 1.22?

[raveit65]

Normally, we support only one stable branch because our manpower is limited. But in the past @monsta did support 1.18 a bit longer after the 1.20 release. But this was his personal decision! Well, 1.22 is very fresh and not announced, so i am not against to make new point releases for 1.20. But , honestly this have to be done by someone other than me..... My plan is to make holidays from Mate after the heavy workload of the last year ;)

[yetist]

@gm10

Please see https://github.com/mate-desktop/mozo/pull/50#issuecomment-469951769 , now you can test mozo 1.20, 1.22 and master, I hope these have been fixed.

[gm10]

@yetist thanks for trying, but that still crashes when moving to a higher level in the hierarchy. As I mentioned previously, the crucial part (for 1.20 at least) was that you need to pass the full path in order to move. That's why I had changed: self.__addXmlMove(xml_root, '/'.join(old_path), '/'.join(new_path), dom) to self.__addXmlMove(xml_root, '/'.join(path), '/'.join(new_path), dom)

I wonder if my #47 here doesn't work in 1.22, then you could just merge #47 + #51. I still haven't found time to compile 1.22 myself unfortunately, it's on the list now that you've released it...

[yetist]

If #47 is better than #50 , I can close #50. Then #47 merge into 1.20, 1.22 and master, #51 merge into 1.22 and master? Is that right?

[gm10]

@yetist that's the idea, but only if you can confirm yourself that it's actually working on 1.22 as well. As I said, I haven't had a chance to check out 1.22 myself yet unfortunately.

[yetist]

I tested the master branch, it worked fine with #47 and #51. But sadly, 1.20 can't apply #47, 1.21+ use space for indent, but 1.20 use Tab for indent, after manually modifying the patch, 1.20 works fine. In this case, we have no way to use the same patch on 1.20 and 1.22+, and we can only commit it separately.

[yetist]

I suggest you merge #51 into #47 as one commit, which will be merged into 1.22 and master.

Then open a new PR for 1.20.

[gm10]

@yetist sure, can do. Btw instead of #51 and #49 it would have been much simpler to flip the argument order on the function. ;)

edit: Done.

[yetist]

Not sure if the change of #49 is necessary. If it is not necessary, is it not necessary to do #51?

If this is the case, you can revert the #49 in #47. I think it's better to keep the minimum changes to the original code.

[gm10]

@yetist I'm entirely relying on you in that respect. I have no idea why it would be necessary, I was assuming API changes. Guess I'll have to get my 1.22 on and check for myself.

[yetist]

Ok, there is no time today, I will test it tomorrow.

[gm10]

@yetist alright, I checked this on 1.22, works fine without #49 and #51, reverted #49, you can close #51.

However, while testing I noticed that trying to create a new menu in the 1.22 mozo results in this:

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/Mozo/MainWindow.py", line 312, in on_new_menu_button_clicked
    GLib.timeout_add(100, self.waitForNewMenuProcess, process, parent.menu_id, file_path)
AttributeError: 'TreeDirectory' object has no attribute 'menu_id'

This also happens with master so unless I have a build error there seems to be more that needs fixing in 1.22.

[yetist]

Test results: drag & drog menu categories into another (sub)menu categories is ok, use "Move Up" "Move Down" button move menu categories is ok. But use "Move Up" or "Move Down" button to move menu entry get error:

Traceback (most recent call last):
  File "/home/yetist/checkout/03-mate-desktop/mozo/Mozo/MainWindow.py", line 614, in on_move_down_button_clicked
    self.editor.moveItem(item, item.get_parent(), after=after)
  File "/home/yetist/checkout/03-mate-desktop/mozo/Mozo/MenuEditor.py", line 756, in moveItem
    self.__positionItem(parent, item, before=before, after=after)
  File "/home/yetist/checkout/03-mate-desktop/mozo/Mozo/MenuEditor.py", line 760, in __positionItem
    contents = self.getContents(parent)
  File "/home/yetist/checkout/03-mate-desktop/mozo/Mozo/MenuEditor.py", line 243, in getContents
    item_iter = item.iter()
AttributeError: 'TreeEntry' object has no attribute 'iter'

[yetist]

@yetist alright, I checked this on 1.22, works fine without #49 and #51, reverted #49, you can close #51.

However, while testing I noticed that trying to create a new menu in the 1.22 mozo results in this:

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/Mozo/MainWindow.py", line 312, in on_new_menu_button_clicked
    GLib.timeout_add(100, self.waitForNewMenuProcess, process, parent.menu_id, file_path)
AttributeError: 'TreeDirectory' object has no attribute 'menu_id'

This also happens with master so unless I have a build error there seems to be more that needs fixing in 1.22.

Please use this patch to fix create "New Menu" and "New Item" error:

diff --git a/Mozo/MainWindow.py b/Mozo/MainWindow.py
index 849de09..57007e2 100644
--- a/Mozo/MainWindow.py
+++ b/Mozo/MainWindow.py
@@ -309,7 +309,7 @@ class MainWindow:
             parent = menus[iter][2]
         file_path = os.path.join(util.getUserDirectoryPath(), util.getUniqueFileId('mozo-made', '.directory'))
         process = subprocess.Popen(['mate-desktop-item-edit', file_path], env=os.environ)
-        GLib.timeout_add(100, self.waitForNewMenuProcess, process, parent.menu_id, file_path)
+        GLib.timeout_add(100, self.waitForNewMenuProcess, process, parent.get_menu_id(), file_path)

     def on_new_item_button_clicked(self, button):
         menu_tree = self.tree.get_object('menu_tree')
@@ -322,7 +322,7 @@ class MainWindow:
             parent = menus[iter][2]
         file_path = os.path.join(util.getUserItemPath(), util.getUniqueFileId('mozo-made', '.desktop'))
         process = subprocess.Popen(['mate-desktop-item-edit', file_path], env=os.environ)
-        GLib.timeout_add(100, self.waitForNewItemProcess, process, parent.menu_id, file_path)
+        GLib.timeout_add(100, self.waitForNewItemProcess, process, parent.get_menu_id(), file_path)

     def on_new_separator_button_clicked(self, button):
         item_tree = self.tree.get_object('item_tree')

[yetist]

But use "Move Up" or "Move Down" button to move menu entry get error:

It seems that the #49 is necessary, and if apply #49, it will be all right.

[yetist]

The last issue I found is that some times it will show the errors:

** (mozo:28888): CRITICAL **: 10:24:33.956: matemenu_tree_get_root_directory: assertion 'tree->loaded' failed

EDIT: If I use this patch, I will no longer see this error.

diff --git a/Mozo/MenuEditor.py b/Mozo/MenuEditor.py
index e0b7572..f94d591 100644
--- a/Mozo/MenuEditor.py
+++ b/Mozo/MenuEditor.py
@@ -88,6 +88,7 @@ class MenuEditor(object):
             with codecs.open(getattr(self, menu).path, 'w', 'utf-8') as f:
                 f.write(getattr(self, menu).dom.toprettyxml())
         if not from_loading:
+            self.load()
             self.__loadMenus()

     def quit(self):

[yetist]

@gm10

So, if I use the follow patch based on master, everything works fine, can you try it?

diff --git a/Mozo/MainWindow.py b/Mozo/MainWindow.py
index 19e2894..4ca5b84 100644
--- a/Mozo/MainWindow.py
+++ b/Mozo/MainWindow.py
@@ -309,7 +309,7 @@ class MainWindow:
             parent = menus[iter][2]
         file_path = os.path.join(util.getUserDirectoryPath(), util.getUniqueFileId('mozo-made', '.directory'))
         process = subprocess.Popen(['mate-desktop-item-edit', file_path], env=os.environ)
-        GLib.timeout_add(100, self.waitForNewMenuProcess, process, parent.menu_id, file_path)
+        GLib.timeout_add(100, self.waitForNewMenuProcess, process, parent.get_menu_id(), file_path)

     def on_new_item_button_clicked(self, button):
         menu_tree = self.tree.get_object('menu_tree')
@@ -322,7 +322,7 @@ class MainWindow:
             parent = menus[iter][2]
         file_path = os.path.join(util.getUserItemPath(), util.getUniqueFileId('mozo-made', '.desktop'))
         process = subprocess.Popen(['mate-desktop-item-edit', file_path], env=os.environ)
-        GLib.timeout_add(100, self.waitForNewItemProcess, process, parent.menu_id, file_path)
+        GLib.timeout_add(100, self.waitForNewItemProcess, process, parent.get_menu_id(), file_path)

     def on_new_separator_button_clicked(self, button):
         item_tree = self.tree.get_object('item_tree')
diff --git a/Mozo/MenuEditor.py b/Mozo/MenuEditor.py
index 17315da..f94d591 100644
--- a/Mozo/MenuEditor.py
+++ b/Mozo/MenuEditor.py
@@ -88,6 +88,7 @@ class MenuEditor(object):
             with codecs.open(getattr(self, menu).path, 'w', 'utf-8') as f:
                 f.write(getattr(self, menu).dom.toprettyxml())
         if not from_loading:
+            self.load()
             self.__loadMenus()

     def quit(self):
@@ -425,11 +426,9 @@ class MenuEditor(object):
         if menu.get_parent() != new_parent:
             dom = self.__getMenu(menu).dom
             path = self.__getPath(menu)
-            root_path = path[0]
-            xml_root = self.__getXmlMenu(root_path, dom.documentElement, dom)
-            old_path = path[1:]
-            new_path = self.__getPath(new_parent)[1:] + [menu.get_menu_id()]
-            self.__addXmlMove(xml_root, '/'.join(old_path), '/'.join(new_path), dom)
+            xml_root = self.__getXmlMenu(path[0], dom.documentElement, dom)
+            new_path = self.__getPath(new_parent) + [menu.get_menu_id()]
+            self.__addXmlMove(xml_root, '/'.join(path), '/'.join(new_path), dom)
         self.__positionItem(new_parent, menu, before, after)
         self.__addUndo([self.__getMenu(new_parent),])
         self.save()
@@ -578,6 +577,8 @@ class MenuEditor(object):
         return None

     def __getXmlMenu(self, path, element, dom):
+        if isinstance(path, str):
+            return element
         for name in path:
             found = self.__getXmlMenuPart(element, name)
             if found is not None:

[gm10]

@yetist I couldn't reproduce the error you were seeing at all but I also don't see any harm in applying your change so I added it as https://github.com/mate-desktop/mozo/pull/47/commits/f91a08eee5610b429d8e09b968f3fcae24fd214a.

I can confirm your new item fix works, but I'll make that a separate PR, not good to mix unrelated changes in the same commit if you can avoid it.

[yetist]

@gm10 I think the c8e6daaaebf1bbeb456b61d55fd5aded6e4f5fcf is wrong, if #47 with this, menu item can't move with "Move Up/Down" button, code in master branch can.

[gm10]

@yetist this is the funniest PR I ever did, it never stops. To think I originally just wanted to patch a single line of code to prevent a crash. ;)

Ok, reverted the revert, squashed the whole thing, I tried dragging and dropping and button moving everything from menues over items to separators, seems to all work now.