Add optional image to connectors and cables by kvid · Pull Request #153 · wireviz/WireViz

kvid · 2020-08-13T17:29:50Z

Add optional image to connectors and cables (#153)

This image, with an optional caption below, is displayed in the lower
section of the connector/cable node in the diagram - just above the
notes if present.

This solves the basic part of issue #27, and is a continuation of
PR #137 that was closed due to changes in the base branch.

Adding helper function for each of these with a leading <tdX> tag that instructs nested_html_table() to inject attributes to the <td> tag.

kvid · 2020-08-13T17:58:35Z

Edit: This comment is obsolete.
Scaling can be tested e.g. like this for scaling the plug uniformly within a fixed 100x100 cell (obtaining a smaller image), and scaling the cable to fill available space in both directions within a cell with 200 as minimum height (obtaining a larger and distorted image):

------------------------------ examples/ex08.yml ------------------------------
index 8437aab..75b58a3 100644
@@ -8,6 +8,8 @@ connectors:
     pinlabels: [Dot, Dash, Ground]
     show_pincount: false
     image: ../resources/stereo-phone-plug-TRS.png
+    image_scale: true
+    image_size: [100, 100, fixed]
     caption: Tip, Ring, and Sleeve
 
 cables:
@@ -19,6 +21,8 @@ cables:
     wirecount: 3
     shield: SN
     image: ../resources/cable-WH+BN+GN+shield.png
+    image_scale: both
+    image_size: [0, 200]
     caption: Cross-section
 
 connections:

Edit: The diff above was adjusted to fit a force-pushed minor change of the example. I don't think such scalings are useful in the example harness, but only for demonstrating scaling.

- Stereo phone plug (a slight modification of a public domain image from https://openclipart.org/detail/192396/headphones-connctor ). - Cable cross-section drawn to match the wire colors in example 08. - Make the cable jacket and shield colors match the cross-section. - Images for embedding in the connector and cable nodes are stored in a new resources folder.

It specifies how an image will use any extra space available in its cell. Allowed values are one of these strings: - FALSE : keep image its natural size. (Default) - TRUE : scale image uniformly to fit. - WIDTH : expand image width to fill. - HEIGHT : expand image height to fill. - BOTH : expand both image width and height to fill. The value is sent to Graphviz as a scale attribute to the <img> tag. Note that there is normally no extra height in the image cell.

It is a list containing minimum width and minimum height of the image cell. To obtain more available space in the image cell, this size must be set greater than the natural size of the image.

When True, enclose the image cell in a table without borders to avoid narrow borders when the fixed width is less than the node width.

src/wireviz/DataClasses.py

17o2 · 2020-08-14T08:04:35Z

src/wireviz/Harness.py

                     html_line_breaks(connector.subtype),
                     f'{connector.pincount}-pin' if connector.show_pincount else None,
-                     connector.color, '<!-- colorbar -->' if connector.color else None],
+                     connector.color, html_colorbar(connector.color)],


I am not against this change, but it probably should be a separate PR since it is unrelated to the image issue

I agree. I put it here as a separate commit just to demonstrate the power of the new functionality.

src/wireviz/Harness.py

src/wireviz/wv_helper.py

examples/ex08.yml

src/wireviz/DataClasses.py

src/wireviz/wv_helper.py

@formatc1702

@formatc1702 requested changes in his review of wireviz#153: - Lowercase attribute values - Nested image attributes - Avoid sending the whole node object as argument to helper functions - Simplify html_size_attr() - Move the resources folder

src/wireviz/DataClasses.py

17o2 · 2020-08-15T08:43:41Z

I'm very happy with how this came out! See my only comment regarding a little cheat sheet for the [not so obvious] scale attribute, and the missing (?) Optional flags.

Since the end result of this PR is very straightforward (mainly just adding lines, not many changes to existing ones), I would like to squash it all into one commit if that is OK with you. Please let me know! Thanks a lot for this contribution.

src/wireviz/DataClasses.py

17o2 · 2020-08-15T09:17:16Z

Please see my comments in 89b730b regarding the documentation.

martonmiklos · 2020-08-28T17:50:51Z

You are right, I just wanted to make a quick test and using the output file was a quick but bad idea.
I apologise for the noise and many thanks for your time and work spent on this project!

kvid · 2020-08-29T19:25:26Z

Sorry, for personal reasons I was not able to work on the project this week, and now I won't have time to test your new feature until after my holidays (ending in two weeks). I look forward to trying this out.
I don't want to rush things so I will try out your code once I have time! :)

Don't worry. You don't have to promise anything. You decide when things are ready for release. There are always things in our lives (like family, friends, etc.) that are far more important that a software project.

I can understand you are not very keen on adding another library dependency, but the non-library alternative looks ugly and might be a challenge to maintain for all supported image types. What do you think? Should I call the PIL (Python Image Library) if it is installed, or assume 1:1 aspect ratio otherwise?

Using PIL is fine.

I had already pushed the aspect_ratio() function that uses PIL, but fall back to 1:1 aspect ratio if failing for any reason.

My only request right now would be to add PIL to requirements.txt and setup.py as a new dependency.

Are you sure you want to require PIL to be installed? My current implementation catch the ModuleNotFoundError if PIL is not found and fall back to 1:1 aspect ratio in such a case.

The code above is working as long as I use absolute path to the image. What is the easiest way to translate a relative image path inside a YAML file to the correct absolute path?

Maybe get the YAML's absolute path and interpret the image path as relative to that? Can't give a code example right now but pathlib should make this relatively painless.

I had already done that. I just needed a clean way to pass the directory of the .gv file into the Image dataclass, and solved that by injecting it as a value in the yaml_data.

This probably means you are right about a zero width or height value is regarded equal as if it was not specified, and therefore it should be safe to only test if image.width instead of if image.width is not None. I'm not 100 % sure, because I've only looked into a fraction of the Graphviz source code, but it's a strong indicator. Do you have more knowledge about this subject?

No idea, I simply assumed that 0 didn't make sense ;-) It was just a guess.

I decided to trust the strong indicator I found and pushed your suggested changes.

It is only used during initialization in the __post_init__() function and does not need to be a proper attribute of the Image dataclass. https://docs.python.org/3/library/dataclasses.html#init-only-variables

src/wireviz/DataClasses.py

17o2 · 2020-10-09T15:07:34Z

One last thing, and this PR will finally be merged!
Please add PIL / pillow to both requirements.txt and setup.py (under install_requires=[).
I think it's more elegant to have all dependencies installed, even if a user might not use images. Your error catching is still great, if for some reason PIL is uninstalled.. that way WireViz will fall back to the default 1:1 ratio.
Thanks again for this great addition to the project!

kvid · 2020-10-10T02:22:05Z

One last thing, and this PR will finally be merged!
Please add PIL / pillow to both requirements.txt and setup.py (under install_requires=[).
I think it's more elegant to have all dependencies installed, even if a user might not use images. Your error catching is still great, if for some reason PIL is uninstalled.. that way WireViz will fall back to the default 1:1 ratio.
Thanks again for this great addition to the project!

I have implemented your request, but after testing it, I have some questions:

Why do you want the run-time dependencies both in requirements.txt and in setup.py when the latter seems to be enough?

The first line of requirements.txt contains ., and that means, AFAIK, that setup.py is executed to find dependencies specified there, and then adding the same dependencies again from the remaining lines of requirements.txt will not change anything.
The last line of requirements.txt contains setuptools, but if that is not already installed before running pip install -r requirements.txt, then it will fail running setup.py when processing the first line containing ..
Moving setuptools above the . in requirements.txt will still fail if setuptools is not already installed, probably because all dependencies are processed before running any installation.
Why do you really need requirements.txt in the repo at all? it does not seem to be used by the project installation instructions.
Maybe the installation instructions should have an initial pip install setuptools to make sure that is installed before running setup.py?

17o2 · 2020-10-10T09:51:07Z

TBH, I am not too familiar with the processes behind pip, setuptools, etc.
I have seen a lot of projects using both, and this post for example, seems to say that, while there is a way to reduce the duplicate references, if is not unreasonable to keep both files. That's why, for now, I'd like to go the safe route and keep both, too.
In the future, we could open a new issue/PR to optimize the setup process, if it is deemed necessary...

kvid · 2020-10-10T12:30:06Z

TBH, I am not too familiar with the processes behind pip, setuptools, etc.
I have seen a lot of projects using both, and this post for example, seems to say that, while there is a way to reduce the duplicate references, if is not unreasonable to keep both files. That's why, for now, I'd like to go the safe route and keep both, too.
In the future, we could open a new issue/PR to optimize the setup process, if it is deemed necessary...

I read the same post yesterday that you link to, and I concluded there are different opinions about the reason behind requirements.txt:

Some argue that it's used for installing everything needed for module development without installing the module, but then it doesn't make sense to include . (that does install this module).
Some argue to include just . and nothing else to let setup.py do the installation job, but then why store and use the requirements.txt at all?
Some argue to store the output from pip freeze into requirements.txt as a documentation on the exact development environment, but in this project we cannot expect all developers to use the exact same set of versions.
In any case it doesn't make sense to include both . and setuptools in the same requirements.txt, as far as I can see.

I will update both files when that is your request, even though I don't like this double specification of the exact same information. A new issue is a good idea, and I will probably create that later today.

17o2 · 2020-10-10T14:26:54Z

Yes, probably there are two of these views in our source code right now, clashing.

I think the first argument, of removing . from requirements.txt is the most reasonable, so let's do that for now. Since I am unsure of any other ramifications of emptying/removing requirements.txt at this point, let's keep the duplicate dependency info for now and discuss in a new issue.
Thanks!

17o2 · 2020-10-11T11:48:00Z

Please let me know when you've added the PIL dependencies (even if they're still duplicated) and implemented your suggestion that I commented on.
#172 can be resolved later once we reach a decision on how to deal with the duplication.

We've talked about this before, but I'd like to know your opinion, just to be sure we agree:
I will probably squash this entire PR into one commit, since it's mostly additions/few modifications of existing code and, while the process to get here was long, the final implementation is very straightforward, so not much information should be lost if squashed. But if you prefer preserving the history, it would not be a problem either. :) Just let me know.

As requested by the owner, it is added to both requirements.txt and setup.py - see also issue wireviz#172 about a possible redundancy/conflict.

kvid · 2020-10-12T03:47:21Z

Please let me know when you've added the PIL dependencies (even if they're still duplicated) and implemented your suggestion that I commented on.
#172 can be resolved later once we reach a decision on how to deal with the duplication.

We've talked about this before, but I'd like to know your opinion, just to be sure we agree:
I will probably squash this entire PR into one commit, since it's mostly additions/few modifications of existing code and, while the process to get here was long, the final implementation is very straightforward, so not much information should be lost if squashed. But if you prefer preserving the history, it would not be a problem either. :) Just let me know.

I have committed these changes locally, but awaits your opinion about adding the exception - see #153 (comment).

I agree #172 can be fully resolved later, but please read my #172 (comment) about removing the conflict before releasing v0.2.

I also agree squashing this entire PR into one commit. Can you use this as commit message, or should I add something more? Suggested commit message:

Add optional image to connectors and cables (#153)

This image, with an optional caption below, is displayed in the lower 
section of the connector/cable node in the diagram - just above the 
notes if present.

This solves the basic part of issue #27, and is a continuation of 
PR #137 that was closed due to changes in the base branch.

17o2 · 2020-10-13T10:15:19Z

I have replied to your comment regarding the exception (--> leave as is, let GraphViz issue a warning).
The quick solution to #172 seems to have settled on removing . from requirements.txt, so that is done as well.
The commit message is perfectly fine! :)

Then let's get rid of that . and close this PR. Thanks.

kvid · 2020-10-14T07:20:07Z

I have replied to your comment regarding the exception (--> leave as is, let GraphViz issue a warning).

OK, I have now pushed my commits without the exception.

The quick solution to #172 seems to have settled on removing . from requirements.txt, so that is done as well.

I'm not sure I would say the solution "has settled" when only one contributor has responded and his comment is not very conclusive, but rather a bit vague... 😞

The commit message is perfectly fine! :)

Should I do the squash rebase to keep my authorship or will that be unchanged even if you do the squashing?

Then let's get rid of that . and close this PR. Thanks.

IMHO, removing a conflict in requirements.txt is out of scope for this PR. I don't like to hide such a change in an unrelated squash commit.

17o2 · 2020-10-14T07:29:07Z

The quick solution to #172 seems to have settled on removing . from requirements.txt, so that is done as well.

I'm not sure I would say the solution "has settled" when only one contributor has responded and his comment is not very conclusive, but rather a bit vague... 😞

True. But nothing is stopping us from adding pillow to both files right now, to close this PR, right?

The commit message is perfectly fine! :)

Should I do the squash rebase to keep my authorship or will that be unchanged even if you do the squashing?

AFAIK, authorship should be preserved. Let's try. If it does not work, I will roll back, you squash, and it will be a learning experience ;)

Then let's get rid of that . and close this PR. Thanks.

IMHO, removing a conflict in requirements.txt is out of scope for this PR. I don't like to hide such a change in an unrelated squash commit.

Fine, I agree. Let's keep things transparent and split it into a new PR.

kvid · 2020-10-14T10:43:16Z

The quick solution to #172 seems to have settled on removing . from requirements.txt, so that is done as well.

I'm not sure I would say the solution "has settled" when only one contributor has responded and his comment is not very conclusive, but rather a bit vague... 😞

True. But nothing is stopping us from adding pillow to both files right now, to close this PR, right?

No, I believe the current state of this branch can be used. I have checked the raw logs from the latest 3.7 and 3.8 tests, and did not find any warning outputs. That indicates importing from PIL went OK when aspect_ratio() was called for ex08.yml. Ideally, we should test installing from scratch at all supported platforms and Python versions, but I currently are not able to do that. What are the test plans before a release?

Since this PR is only a partial solution to #27, I assume you keep #27 open so the discussions about more advanced integration of images are not so easily forgotten.

The commit message is perfectly fine! :)

OK

Should I do the squash rebase to keep my authorship or will that be unchanged even if you do the squashing?

AFAIK, authorship should be preserved. Let's try. If it does not work, I will roll back, you squash, and it will be a learning experience ;)

OK

Then let's get rid of that . and close this PR. Thanks.

IMHO, removing a conflict in requirements.txt is out of scope for this PR. I don't like to hide such a change in an unrelated squash commit.

Fine, I agree. Let's keep things transparent and split it into a new PR.

Good. That was also one of the reasons behind the #172 issue.

17o2 · 2020-10-14T10:52:08Z

I have checked the raw logs from the latest 3.7 and 3.8 tests, and did not find any warning outputs. [...] Ideally, we should test installing from scratch at all supported platforms and Python versions, but I currently are not able to do that. What are the test plans before a release?

Honestly, right now I am happy with seeing the passing builds that you linked to. There's no plan to do exhaustive testing at this point; I'd rather release first and wait for eventual bug reports... we are still at a very early stage and, unless we can somehow automate more exhaustive testing (new issue?), I will not worry too much about it.

While aiming for quality is a noble goal, I'd like to focus on moving things along quickly, the way we have been (at least before my holiday). I hope you understand.

17o2 · 2020-10-14T14:09:39Z

4782da4 was squashed and merged, preserving your authorship. Thanks for this valuable addition to WireViz! 🎉

This image, with an optional caption below, is displayed in the lower section of the connector/cable node in the diagram - just above the notes if present. This solves the basic part of issue wireviz#27, and is a continuation of PR wireviz#137 that was closed due to changes in the base branch.

kvid added 3 commits August 13, 2020 19:14

Add an image attribute to both Connectors and Cables

43ec3bf

Add a caption attribute to both Connectors and Cables

c8c4005

Remove border between image and caption

0cb7118

Adding helper function for each of these with a leading <tdX> tag that instructs nested_html_table() to inject attributes to the <td> tag.

kvid changed the title ~~Issue27 embedded image~~ Options for embedding images in connector/cable/bundle nodes in the diagram Aug 13, 2020

kvid mentioned this pull request Aug 13, 2020

Add optional image and caption attributes to Connector and Cable/Bundle nodes #137

Closed

kvid added 4 commits August 13, 2020 22:51

Add an image_size attribute to both Connectors and Cables

41b3f3a

It is a list containing minimum width and minimum height of the image cell. To obtain more available space in the image cell, this size must be set greater than the natural size of the image.

Add fixedsize as third image_size value

3d7f027

When True, enclose the image cell in a table without borders to avoid narrow borders when the fixed width is less than the node width.

kvid force-pushed the issue27-embedded-image branch 2 times, most recently from a3774d5 to 9a65223 Compare August 14, 2020 02:57