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

[bug] hide_disconnected_pins when nothing is connected #257

Closed
egradman opened this issue Nov 12, 2021 · 11 comments · Fixed by #375
Closed

[bug] hide_disconnected_pins when nothing is connected #257

egradman opened this issue Nov 12, 2021 · 11 comments · Fixed by #375
Labels
bug Something isn't working

Comments

@egradman
Copy link

Hi,

When I have hide_disconnected_pins set on a connector that has no connections, I get a syntax error from dot. Looks like an empty table isn't supported.

I realize that this is a super edge-case, but the error message wasn't helpful enough to track down the problem, and it took me a long time to figure out what was going on. I generate some pretty complicated wireviz documents with jinja2, so sometimes I encounter weird edge cases like this!

Love the project. Thanks so much for making it!

@formatc1702
Copy link
Collaborator

formatc1702 commented Nov 15, 2021

Thanks for the input. I am currently refactoring the entire graph generation code, so I will keep this edge case in mind.

Is there a way to see some of the diagrams you are generating, out of sheer curiosity? Feel free to post them in #143.
I've also noticed your obsidian-wireviz repo.. Never used Obsidian myself, but nice to know that WireViz will be supported :-)

@egradman
Copy link
Author

egradman commented Nov 15, 2021 via email

@kvid
Copy link
Collaborator

kvid commented Dec 29, 2023

Another user has just reported the same bug, and I linked the duplicate to this issue. As both bug reports use quite complicated YAML input examples to demonstrate the same bug, I suggest here a much simpler YAML input to provoke the same error:

connectors:
  X1:
    pincount: 2
    hide_disconnected_pins: true

I got this error when running WireViz 0.3.2 with the input above (in Python 3.7.5 and Graphviz 2.40.1 at Ubuntu 18.04):

Error: syntax error in line 15
...
  </table> ...
in label of node X1
Traceback (most recent call last):
  File "wireviz.py", line 270, in <module>
    main()
  File "wireviz.py", line 266, in main
    parse(yaml_input, file_out=file_out)
  File "wireviz.py", line 195, in parse
    harness.output(filename=file_out, fmt=('png', 'svg'), view=False)
  File "../wireviz/Harness.py", line 437, in output
    graph.render(filename=filename, view=view, cleanup=cleanup)
  File "/home/kvid/src/formatc1702/WireViz/wv-env/lib/python3.7/site-packages/graphviz/files.py", line 209, in render
    quiet=quiet)
  File "/home/kvid/src/formatc1702/WireViz/wv-env/lib/python3.7/site-packages/graphviz/backend.py", line 221, in render
    run(cmd, capture_output=True, cwd=cwd, check=True, quiet=quiet)
  File "/home/kvid/src/formatc1702/WireViz/wv-env/lib/python3.7/site-packages/graphviz/backend.py", line 184, in run
    output=out, stderr=err)
graphviz.backend.CalledProcessError: Command '['dot', '-Tpng', '-O', 'single_connector_hide']' returned non-zero exit status 1. [stderr: b'Error: syntax error in line 15 \n... \n  </table> ...\nin label of node X1\n']

@kvid kvid changed the title hide_disconnected_pins when nothing is connected [bug] hide_disconnected_pins when nothing is connected Dec 29, 2023
@martinrieder
Copy link
Contributor

martinrieder commented Jun 4, 2024

There are a few other syntax options for show_*, but only a single one for hide_*. This seems counter-intuitive and might be confusing some users. I would like to argue that the option hide_disconnected_pins should be dropped in favor of a new syntax show_disconnected, which would apply to either connectors or wires or both.

The latter part adds new functionality to wires though, so we might need to discuss this in a new issue. It depends a bit on how this bug would be solved. There was also a similar bug related to loops in issue #263, which got solved by PR #264 by @kvid.

Note that I dropped the ending _pins intentionally to support #331 by @scottbouch. Refer to PR #5 for the original implementation by @elliotmr.

PS: I can confirm this bug. My use case involved using a YAML template that I applied to all connectors. I found this as good practice to show all pins while building the connections list and only hide them when finished.

kvid added a commit that referenced this issue Jun 5, 2024
@kvid kvid added the bug Something isn't working label Jun 5, 2024
@kvid
Copy link
Collaborator

kvid commented Jun 5, 2024

It's a shame this bug has been present for so long without being fixed. It was not complicated at all. @egradman and @martinrieder - Please try my PR #375 and report here if this issue was fixed.

PS: I agree with @martinrieder that his suggestion about renaming the connector attribute deserves a new issue.

@martinrieder
Copy link
Contributor

martinrieder commented Jun 5, 2024

Fine, the option is called hide_disconnected_pins, so you are hiding the pinhtml table. This produces different results for "simple" connectors, since these are treated separately. It is a bit edgy, but it works. Thanks for providing a fix!

connectors:
  X1:
    style: simple
    hide_disconnected_pins: true
connections:
  - - X1

hide_disconnected_pins_simple

connectors:
  X1:
    pincount: 1
    show_name: false
    show_pincount: false
    hide_disconnected_pins: true
connections:
  - - X1

hide_disconnected_pins_anonymous

Note: If the connector is not referenced in the connections section, then it will not be shown at all, accompanied by a warning on the CLI.

@kvid
Copy link
Collaborator

kvid commented Jun 6, 2024

@martinrieder wrote:

Fine, the option is called hide_disconnected_pins, so you are hiding the pinhtml table.

I only hide this table when it's empty to avoid the Graphviz error.

This produces different results for "simple" connectors, since these are treated separately.

  • Please provide a YAML input with only a "simple" connector that produces different results with this fix than with v0.4 to prove your statement.
  • Simple connectors doesn't show the pin table, so my fix doesn't affect simple connectors - they produce the same output as with v0.4.

It is a bit edgy, but it works.

I chose to not output an empty table that produced an error. Another choice could be to insert a dummy cell in empty tables, but that would increase the space of the connector node. Is that better? Please suggest alternatives.

Thanks for providing a fix!

connectors:
  X1:
    style: simple
    hide_disconnected_pins: true
connections:
  - - X1
  • The hide_disconnected_pins: true has no effect on a simple connector because it doesn't show the pin table at all.
  • The collapsed size of this connector node is due to an empty type attribute. If you add type: " " or some other non-empty text, you can see that the node increases in size.
  • All these variations produce the same output with my fix as with v0.4.
connectors:
  X1:
    pincount: 1
    show_name: false
    show_pincount: false
    hide_disconnected_pins: true
connections:
  - - X1

This does not collapse in size due to some white-space in the otherwise empty cell in the connector table. I can remove that white-space, but It'll require code changes several places in the code instead of only at one single place. Another way to make the output of a simple connector more like a connector with all pins hidden, is to insert the similar white-space in the simple connector case, but I haven't tested that alternative for side-effects.

I don't want to make too much out of this fix because PR #251 will change all this code very soon anyway.

Note: If the connector is not referenced in the connections section, then it will not be shown at all, accompanied by a warning on the CLI.

That is as designed. See the syntax description about this.

@martinrieder
Copy link
Contributor

martinrieder commented Jun 6, 2024

This produces different results for "simple" connectors, since these are treated separately.

  • Please provide a YAML input with only a "simple" connector that produces different results with this fix than with v0.4 to prove your statement.

I did exactly this, but the collapsed simple connector is easy to miss on a white background:
Screenshot_20240606-050935~2.png

I chose to not output an empty table that produced an error. Another choice could be to insert a dummy cell in empty tables, but that would increase the space of the connector node. Is that better? Please suggest alternatives.
[...]

  • The collapsed size of this connector node is due to an empty type attribute. If you add type: " " or some other non-empty text, you can see that the node increases in size.

There is need to have a dummy table: It would be less confusing if the whitespace was internally set as a default. If users wanted to override this, they would have to define none explicitly.

Another way to make the output of a simple connector more like a connector with all pins hidden, is to insert the similar white-space in the simple connector case, but I haven't tested that alternative for side-effects.

I am thinking the same, though it would need to be solved in another issue. There are some more important things to tackle than a "missing whitespace". ;-)

connectors:
  X1:
    pincount: 1
    show_name: false
    show_pincount: false
    hide_disconnected_pins: true
connections:
  - - X1

This does not collapse in size due to some white-space in the otherwise empty cell in the connector table. I can remove that white-space, but It'll require code changes several places in the code instead of only at one single place.

I do not see any benefit in this. Please leave it as is.

I don't want to make too much out of this fix because PR #251 will change all this code very soon anyway.

I am being pedantic, so please don't get me wrong. I just wanted to highlight the corner case by providing this example.

It is a bit edgy, but it works.

I meant to say: The described bug is fixed. End of story. ^^

@kvid
Copy link
Collaborator

kvid commented Jun 6, 2024

We seem to agree that the reported bug is fixed, so I will not add more to my fix now. However, I just want you @martinrieder to understand that my fix has absolutely no effect on simple connectors. If you try that YAML input example with WireViz v0.4, you will see an identical collapsed output as with my fixed version. You can also remove the hide_disconnected_pins: true for simple connectors without changing the output in any way.

@martinrieder
Copy link
Contributor

martinrieder commented Jun 6, 2024

This produces different results for "simple" connectors, since these are treated separately.

  • Please provide a YAML input with only a "simple" connector that produces different results with this fix than with v0.4 to prove your statement.

I did exactly this, but the collapsed simple connector is easy to miss on a white background:

@kvid sorry for the confusion, but we are on the same page. Comparing before and after fixing only has no effect on the simple connectors, of course.
This was a misunderstanding because I did not clearly state what is different. I meant to only compare simple vs. standard connectors.
That is out-of-scope for the reported bug, so I took the idea to insert some whitespace by default (for simple connectors) over here: #223 (comment)

kvid added a commit that referenced this issue Jun 7, 2024
@kvid kvid linked a pull request Jun 22, 2024 that will close this issue
@kvid
Copy link
Collaborator

kvid commented Jul 13, 2024

Closed after releasing v0.4.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants