Skip to content
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

Allow using dot parser in a server environment #108

Open
flying-sheep opened this issue Sep 13, 2022 · 10 comments
Open

Allow using dot parser in a server environment #108

flying-sheep opened this issue Sep 13, 2022 · 10 comments

Comments

@flying-sheep
Copy link
Contributor

flying-sheep commented Sep 13, 2022

I would like to use the dot parser in a server environment, but just importing it results in initializing a GTK window.

It would be great to at least initialize that lazily or even better: Splitting the parser off into another package that can be imported without any GTK/GI dependencies

/opt/hostedtoolcache/Python/3.8.14/x64/lib/python3.8/site-packages/gvrender/matplotlib.py:21: in <module>
    from xdot.dot.parser import XDotParser
/opt/hostedtoolcache/Python/3.8.14/x64/lib/python3.8/site-packages/xdot/__init__.py:26: in <module>
    from . import ui
/opt/hostedtoolcache/Python/3.8.14/x64/lib/python3.8/site-packages/xdot/ui/__init__.py:3: in <module>
    from .window import DotWidget, DotWindow
/opt/hostedtoolcache/Python/3.8.14/x64/lib/python3.8/site-packages/xdot/ui/window.py:40: in <module>
    from . import actions
/opt/hostedtoolcache/Python/3.8.14/x64/lib/python3.8/site-packages/xdot/ui/actions.py:66: in <module>
    class NullAction(DragAction):
/opt/hostedtoolcache/Python/3.8.14/x64/lib/python3.8/site-packages/xdot/ui/actions.py:70: in NullAction
    _tooltip_window = Gtk.Window(Gtk.WindowType.POPUP)
/opt/hostedtoolcache/Python/3.8.14/x64/lib/python3.8/site-packages/gi/overrides/Gtk.py:519: in __init__
    raise RuntimeError(
E   RuntimeError: Gtk couldn't be initialized. Use Gtk.init_check() if you want to handle this case.
@jrfonseca
Copy link
Owner

Commit c4fad2f would fix this. However I'm concerned about all the projects which rely upon xdot.Dotwindow existing...

@jrfonseca
Copy link
Owner

I'm wrong. There are still dot -> ui dependencies..

$ git grep ui xdot/dot/
xdot/dot/parser.py:from ..ui.colors import lookup_color
xdot/dot/parser.py:from ..ui.pen import Pen
xdot/dot/parser.py:from ..ui import elements

For this to work, we'd need to yank out the XdotParser out of xdot/dot. Do you want to parse xdot annotations too, or just plain dot?

BTW, separate package is not something I'm keen on, as another package would increase the maintenance burden, and I don't have enough time for all my open-source projects already.

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Sep 13, 2022

There are still dot -> ui dependencies..

well, those can maybe be moved and then re-exported from their old location:

  • Pen and lookup_color don’t depend on anything and can easily be moved elsewhere.

  • elements would be a more involved refactoring, I see two options:

    1. make sure only _draw methods use GTK/GDK stuff and import things inline (first line of each _draw method)

      as long as one doesn’t call _draw, no GTK/GDK needs to be installed.

    2. move Shape, TextShape, … to another location, remove the _draw method and add subclasses with the same names and the _draw method to ui:

      # file xdot/elements.py or so:
      class Shape:
          ... # everything except for `_draw`
      
      class TextShape(Shape):
          ...  # likewise
      
      # file xdot/ui/elements.py or so:
      from gi.repository import Gdk
      
      from .. import elements as base_elements
      
      class Shape(base_elements.Shape):
          def _draw(...): raise NotImplementedError(...)  # as before
      
      class TextShape(Shape, base_elements.TextShape):
          def _draw(...): ...

I'm concerned about all the projects which rely upon xdot.Dotwindow existing...

There’s options:

  1. Rely on PEP 562, add xdot.__getattr__(name) and drop support for Python <3.7:

    def __getattr__(name: str):
        if name == "Dotwindow":
            from .ui import Dotwindow
            return Dotwindow
        return AttributeError(f"module {__name__!r} has no attribute {name!r}")

    If doing this, you should also add a requires_python='>=3/7' to setup.py so Python 3.6 users get an appropriate error when trying to install this package from PyPI.

  2. Create a function xdot.Dotwindow(*args, **kwards):

    def Dotwindow(*args, **kw):
        from .ui import Dotwindow as DW
        return DW(*args, **kw)

    Disadvantage: people can’t use isinstance or subclass the top level name since it points to a function, not a class.

jrfonseca added a commit that referenced this issue Sep 14, 2022
@jrfonseca
Copy link
Owner

Thanks. PEP 562 is a great suggestion. I followed it while still keeping backwards compatibility for older Python.

I moved XDotParser into ui and therefore removing all ui dependencies from xdot.dot. Does this meet your needs?

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Sep 14, 2022

You might have read my comment above before I edited it. There’s still dependencies from dot to ui.

Using PEP 562 in ui/__init__.py as well would make the imports from ..ui.colors import lookup_color and from ..ui.pen import Pen work, but not from ..ui import elements since that one does GTK imports and calls itself. If you want I could make a PR that implements one of the strategies in my above comment, but you’d have to decide for strategy i or ii first.

@jrfonseca
Copy link
Owner

There’s still dependencies from dot to ui.

Are you sure? I removed them all now, in branch issue-108:

$ git grep '\<ui\>' xdot/dot
$ DISPLAY=dummy python3 -c 'from xdot.dot.parser import DotParser'

Are you sure you're testing the tip of branch issue-108?

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Sep 14, 2022

Aaah, I see the misconception. I just said “the dot parser” instead of specifying that I’m talking about the XDotParser. The traceback I posted shows that I do from xdot.dot.parser import XDotParser but I should have been more specific.

I want to use that one specifically because it extracts information about shapes and the pen.

My idea was to divorce those abstract drawing-related pieces of information from xdot.py’s concrete GTK drawing implementation.

That way, one can use it to draw to a different kind of canvas.

@jrfonseca
Copy link
Owner

I see.. Yes, that definetly requires more work.

One solution is to encapsulate all ui dependencies into a Factory, whose methods would then create the necessary objects from ui (e.g, createNode, createEdge, createFoo. This would open up the possibility for your code to provide its own objects.

Another simpler solution is to split out XDotParser' handle_foo methods into a base classe. That is, XDotParser just calls handle_foo, and all handling and ui construction is done in the base class.

Both stratergies can be combined. It depends how much you really want to share. The latter seems simpler and more practicle.

That said, I don't have the time to disentangle XDotParser from ui, but I'd accept a PR.

In the mean while I can merge the issue-108 branch, as it does at least allow to parse vanilla dot, and this way the branch doesn't diverge from master

@flying-sheep
Copy link
Contributor Author

That sounds good! Is there other packages that import XDotParser? If so, maybe you should merge this PR into the issue-108 branch before merging that one: #109

@jrfonseca
Copy link
Owner

That sounds good! Is there other packages that import XDotParser?

I hadn't thought about that. Per this GitHub code search, there are a few attempts but they are all very hackish, as XDotParser was never designed to be reused outside xdot.py until now. Furthermore, we'll probably need to change XDotParser to the point that backwards compatibility is unsustainable, so I think it's not worth trying to keep backwards compatibility with XDotParser for now. That said, there's no harm in doing so until XDotParser until it's refactored, so I'll merge it, thanks.

jrfonseca added a commit that referenced this issue Sep 15, 2022
jrfonseca added a commit that referenced this issue Sep 15, 2022
jrfonseca added a commit that referenced this issue Sep 15, 2022
jrfonseca added a commit that referenced this issue Sep 15, 2022
jrfonseca added a commit that referenced this issue Sep 15, 2022
jrfonseca added a commit that referenced this issue Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants