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

[feature] Add tweak option per node #349

Open
kvid opened this issue May 18, 2024 · 6 comments · May be fixed by #357
Open

[feature] Add tweak option per node #349

kvid opened this issue May 18, 2024 · 6 comments · May be fixed by #357

Comments

@kvid
Copy link
Collaborator

kvid commented May 18, 2024

I suggest adding a tweak option for each node (both cables and connectors). It has been on mind many times, but was now triggered again by @martinrieder, and I create a new issue to avoid cluttering the original issue where such a feature would be helpful.

@martinrieder wrote:

[...]
My suggestion on this would take some universal approach: allow the tweak to be defined beneath any cable or connector. Their designators could then automatically be referenced, even for auto-generated ones.

I totally agree. It has been on my mind many times since I wrote my suggestion above, but I haven't given priority to diving into the graphviz package to find how to inject my tweak code for each node. Todays tweak code is very simple using search and replace on the total .gv text output from the graphviz package. To do it per node, it will probably require injecting my code earlier in the process, while each node is generated.

Note that it is possible to define multiple subgraphs with the same name. These will be merged by Graphviz, which allows appending designators to clusters.

That I haven't tried. That sounds like very useful in combination with tweak per node. I have been thinking of adding my own per node specifyer for being included into a common group, but what you describe might be a lower hanging fruit.

Originally posted by @kvid in #270 (comment)

@kvid
Copy link
Collaborator Author

kvid commented May 18, 2024

I wrote:

[...]
To do it per node, it will probably require injecting my code earlier in the process, while each node is generated.

I just realized that I might have been making this problem more complicated than needed. It should be sufficient to accumulate tweaks from each node in the harness and join everything before executing all at once with the total .gv output as we do today.

We just need to find a proper placeholder for the node designator, and replace all usage of this placeholder within each node tweak with the real node designator before joining everything. Such a placeholder should be a short text that is highly unlikely someone wants literally in tweak texts - preferably something illegal to use in the .gv syntax. Otherwise, we can simply add an optional tweak:placeholder so the user must specify the placeholder text when needing one. This latter alternative is also backwards compatible.

Are there also other attributes of a cable or connector that would be useful to have placeholders for? I am mainly thinking about attributes that are not easily known by the YAML author.

@martinrieder
Copy link
Contributor

martinrieder commented May 18, 2024

That sounds still rather complicated. The tweaks would be inserted inside the [ ] brackets before or after the label=<... > HTML table. It comes right after each designator node definition. Why would you want to do any search and replace for this?

Another way that should work, but I have not tested is simply using the same designator multiple times for each tweak. They should be merged by GraphViz in the same fashion that subgraphs are merged when they have identical names.

kvid added a commit that referenced this issue May 22, 2024
@kvid kvid linked a pull request May 22, 2024 that will close this issue
@kvid kvid linked a pull request May 22, 2024 that will close this issue
@martinrieder
Copy link
Contributor

martinrieder commented May 23, 2024

@kvid
Great to see this implemented! Now I understand how you intended to do that search & replace, though I still do not understand what the placeholder would be needed for. I think that it might make sense to have a prefix to an auto-generated designator though... What is the advantage of having a user-defined ID here?

Considering that we use prefixes instead of placeholders, then it could be used to implement the grouping through clusters. This could be a solution to #325 then, if the prefix itself could also be used literally without being replaced. Something like:

GROUP//ID

Because Graphviz has C-style comments, it would ignore everything after // as long as it is not an explicit string .

subgraph clusterGROUP//ID 
{ "GROUP//ID" }

Should work, though I cannot test it currently.

@martinrieder
Copy link
Contributor

That sounds still rather complicated. The tweaks would be inserted inside the [ ] brackets before or after the label=<... > HTML table. It comes right after each designator node definition. Why would you want to do any search and replace for this?

I meant that the tweaks would be applied after the calls to dot.node here https://github.com/wireviz/WireViz/blob/issue349tweak/src%2Fwireviz%2FHarness.py#L292-L298 and here https://github.com/wireviz/WireViz/blob/issue349tweak/src%2Fwireviz%2FHarness.py#L555-L561.

The way that you implemented it applies all the tweaks in a final step. This allows some debugging by comparing before and after tweaking, if needed.

@martinrieder
Copy link
Contributor

@kvid wrote:

Are there also other attributes of a cable or connector that would be useful to have placeholders for? I am mainly thinking about attributes that are not easily known by the YAML author.

Yes and no. I could think of placeholders for metadata like user, file and folder name that a user might like to add in a text label. Date and time stamps are also something useful, though I see that YAML might provide tags for this purpose. I guess that we should discuss this elsewhere.

Note that YAML tags could be used to generate random designators/placeholders!

@kvid
Copy link
Collaborator Author

kvid commented May 24, 2024

@martinrieder wrote:

@kvid Great to see this implemented! Now I understand how you intended to do that search & replace, though I still do not understand what the placeholder would be needed for.

Any presence of the placeholder text will be replaced with the node instance designator. The only reason for making the user select such a placeholder text instead of having a hard coded placeholder, is to avoid the edge case that such a hard coded text is needed literally in any tweak attribute.

See also my newly extended #270 (comment) for a use case with example YAML input using a placeholder.

I think that it might make sense to have a prefix to an auto-generated designator though...

What do you need a prefix for?

What is the advantage of having a user-defined ID here?

See the upper part of this comment. The user is able to choose a placeholder that doesn't conflict with the other tweak text.

Considering that we use prefixes instead of placeholders, then it could be used to implement the grouping through clusters. This could be a solution to #325 then, if the prefix itself could also be used literally without being replaced. Something like:

GROUP//ID

Because Graphviz has C-style comments, it would ignore everything after // as long as it is not an explicit string.

subgraph clusterGROUP//ID 
{ "GROUP//ID" }

I still don't understand how this will work.

Should work, though I cannot test it currently.

kvid added a commit that referenced this issue May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants