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

Port of Menu & Menu editor to ElementTree #4

Merged
merged 8 commits into from May 25, 2013

Conversation

Projects
None yet
2 participants
@ju1ius
Contributor

ju1ius commented Feb 22, 2013

And also some refactoring of the Menu parsing methods: parsing is now done by the xdg.Menu.Parser.parse() method. The xdg.Menu.parse() function is left as a shortcut.
All Menu classes are freed from the parsing job.

There are no unit tests for the MenuEditor class... Do you have some code I could use to test against (and turn to proper unit test while I'm at it) ?

@takluyver

This comment has been minimized.

Owner

takluyver commented Feb 22, 2013

I'll need to take a while to go through all that. I'm glad we opted to do it as a second pull request.

No, I don't have any code that uses MenuEditor, and I'm not aware of any. That's pretty much why it has no tests, and why it still uses from foo import * - I don't want to spend time on something that's not being used. Nullege also doesn't know of anything using that module.

@ju1ius

This comment has been minimized.

Contributor

ju1ius commented Feb 22, 2013

I'm on the process of porting the gtk menu editor found in the gnome-menus package to pyxdg for my project uxdgmenu, so I might be able to write tests along the way, and maybe add a few features when I need them.
It could take some time however...

@ju1ius

This comment has been minimized.

Contributor

ju1ius commented Feb 22, 2013

There's another important thing I want to fix: the parsing methods are incoherent since some of them return an object created from the parsed node, whereas some of them have side-effects (like creating an object and adding it to it's parent).
I want all methods that create an object only return that object, so that they are actually usable as public methods. For example Parser().parse_move(node) should return an xdg.Menu.Move object.

We would then have a clean public API:

# parses a menu file
# returns xdg.Menu.Menu
Parser.parse(filename)
# parses a <Menu> element
# returns xdg.Menu.Menu
Parser.parse_menu(node)
# parses a <Layout> element
# returns xdg.Menu.Layout
Parser.parse_layout(node)
# parses a <Move> element
# returns xdg.Menu.Move
Parser.parse_move(node)
# parses a <Rule> element
# returns xdg.Menu.Rule
Parser.parse_rule(node)
# parses a <Separator> element
# returns xdg.Menu.Separator
Parser.parse_separator(node)

, the rest being internal only.

@takluyver

This comment has been minimized.

Owner

takluyver commented Feb 24, 2013

I'm not entirely comfortable with creating Parser instances when the class isn't storing any state - it seems like unnecessary complexity. Also, the Parser class seems to include a lot of methods that aren't actually related to parsing (like sort - shouldn't that be Menu.sort()?).

I think I'd be inclined to have parsing code as straightforward functions, which extract the relevant details and return Python objects, without side effects, so they're easy to test. parse(filename) should be public, but I'd leave the node-parsing ones private (e.g. _parse_layout) for now, so we're not committed to them as an API. Then they can call other functions, or methods on Menu & MenuEntry to do things like sorting & handling moves.

I haven't examined the menu system in detail, though, so I'm ready to hear counterproposals.

@ju1ius

This comment has been minimized.

Contributor

ju1ius commented Feb 25, 2013

the class isn't storing any state

Yes it is. Look at the parse() method:

        # parse menufile
        self.root = None
        self._merged_files = set()
        self._directory_dirs = set()
        self.cache = MenuEntryCache()

these are instance variables that store parsing state. This state was previously stored in a module global dict called tmp. I think this solution is a lot cleaner.
Also they can't be stored as properties of a Menu object. For example the _merged_files property ensures we don't merge the same file twice. If we were storing this on a Menu instance, we would have to walk the entire AST each time to check that the file hasn't been already merged.

Furthermore, using a class allows to easily switch parsing mode, for example a strict mode in which every parsing error raises an Exception and a normal mode in which the parser tries to recover as much data as it can, etc...

Also, the Parser class seems to include a lot of methods that aren't actually related to parsing

Well technically, the real parser here is the ElementTree parser. Our Parser class is only a translator between the AST created by ElementTree and our xdg.Menu AST. So no methods have anything to do with actual parsing, but every one of them has to do with translating node objects to xdg.Menu AST objects.

So the discussion is really about the few methods that deal with sorting, handling moves, not_only_allocated, etc...
Should they belong to the Menu class ? It seems pretty obvious for the sort method, especially as it is used quite a few times in the MenuEditor class. For the other however I don't know yet. I usually like to think twice before adding a lot of logic to a class that can be instanciated a lot of times, especially if these methods are actually called only during the "parsing" phase... Gonna to check this out.

This will become clearer to me when I've worked more with the MenuEditor...

Btw, thanks for taking time to review all of this! ;-)

@takluyver

This comment has been minimized.

Owner

takluyver commented Feb 25, 2013

Isn't e.g. merged_files a property of the menu object we're building (i.e. 'files that have been merged into this menu structure')?

(Aside: the XML structure and our Menu instance aren't ASTs. An Abstract Syntax Tree represents the structure of a computer program. We build up an AST specifically for the rule, which can be treated as a computable expression. The menu structure is just a tree.)

What's the concern with adding logic to classes that are instantiated a lot of times? As far as I know, it should have a speed or memory cost - the method is only bound when you access it (menu.sort).

@takluyver

This comment has been minimized.

Owner

takluyver commented Mar 7, 2013

OK, having had another look at this, I'm coming round to the idea of a class, but there are still some changes I think it needs:

  • As you said, 'Parser' isn't really accurate, because elementtree is the parser. Maybe it should be called something like XMLMenuBuilder? Method names could be updated to follow suit.
  • Think more about design: e.g. if we create the root Menu instance in __init__, there's no need to have a special case for that in parse_menu.
  • At least sort() seems like it should be a method on the Menu, not the Parser/XMLMenuBuilder. The only other Parser/XMB method it calls is parse_inline, which, while I don't understand what it's doing, doesn't call any Parser/XMB methods itself, so it could be a standalone function, or a method of Menu.
@takluyver

This comment has been minimized.

Owner

takluyver commented Mar 12, 2013

And now, for instance, self.root is only referenced in the main parse() method, so it may as well be a local variable, so that it's clear that it's not modified elsewhere.

@takluyver

This comment has been minimized.

Owner

takluyver commented Mar 18, 2013

Thanks. I'm just testing this, but I see a test failure, both on Python 2.7 and 3.2:

======================================================================
ERROR: test_parse_menu (test-menu.MenuTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/thomas/Code/pyxdg/test/test-menu.py", line 41, in test_parse_menu
    menu = xdg.Menu.parse(self.test_file)
  File "/home/thomas/Code/pyxdg/xdg/Menu.py", line 1124, in parse
    return Parser().parse(filename)
  File "/home/thomas/Code/pyxdg/xdg/Menu.py", line 637, in parse
    menu.sort()
  File "/home/thomas/Code/pyxdg/xdg/Menu.py", line 224, in sort
    submenu.sort()
  File "/home/thomas/Code/pyxdg/xdg/Menu.py", line 276, in sort
    if entry.Directory.DesktopEntry.nodisplay:
AttributeError: 'DesktopEntry' object has no attribute 'nodisplay'

It looks like it might work with ...getNoDisplay() instead, though I haven't tried that.

Also, any thoughts on renaming the Parser class to something like XMLMenuBuilder?

@takluyver

This comment has been minimized.

Owner

takluyver commented Mar 18, 2013

I tried that fix, it did work, so I've made a pull request against your pull request ;-)

@takluyver

This comment has been minimized.

Owner

takluyver commented Mar 26, 2013

Any thoughts on the class name? I'm leaning towards XMLMenuBuilder.

@takluyver

This comment has been minimized.

Owner

takluyver commented May 25, 2013

Alright, I'm going to merge this and rename the class to XMLMenuBuilder. Thanks @ju1ius.

takluyver added a commit that referenced this pull request May 25, 2013

Merge pull request #4 from ju1ius/etree
Port of Menu & Menu editor to ElementTree

@takluyver takluyver merged commit 1ad37f3 into takluyver:master May 25, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment