Skip to content

[WIP]Add connector image alignment #420

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[WIP]Add connector image alignment #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00716d0
Added alignment option to the connector images
martonmiklos Aug 19, 2024
f19fafb
Fix @kvid's suggestions:
martonmiklos Sep 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/wireviz/DataClasses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
PlainText # = Literal['wirecount', 'terminations', 'length', 'total_length']
)
ImageScale = PlainText # = Literal['false', 'true', 'width', 'height', 'both']
ImagePosition = PlainText # = Literal['below', 'above', 'left', 'right']

# Type combinations
Pin = Union[int, PlainText] # Pin identifier
Expand Down Expand Up @@ -86,6 +87,7 @@ class Image:
height: Optional[int] = None
fixedsize: Optional[bool] = None
bgcolor: Optional[Color] = None
position: Optional[ImagePosition] = None
Copy link
Collaborator

@kvid kvid Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
position: Optional[ImagePosition] = None
position: ImagePosition = "below"

I see no need for allowing a None value here. When a None value always should be treated equally as a "below" value, then it's better to use the latter as the default value explicitly.

# Contents of the text cell <td> just below the image cell:
caption: Optional[MultilineHypertext] = None
# See also HTML doc at https://graphviz.org/doc/info/shapes.html#html
Expand Down
45 changes: 37 additions & 8 deletions src/wireviz/Harness.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,12 @@ def create_graph(self) -> Graph:
connector.ports_left = True # Use left side pins.

html = []

image_rows = []
if connector.image and connector.image.position != 'above':
image_rows = [[html_image(connector.image)],
[html_caption(connector.image)]]

Comment on lines +194 to +199
Copy link
Collaborator

@kvid kvid Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
image_rows = []
if connector.image and connector.image.position != 'above':
image_rows = [[html_image(connector.image)],
[html_caption(connector.image)]]
image_rows = [
[html_image(connector.image)],
[html_caption(connector.image)],
]
  • No need for the if test
  • Black formatting

# fmt: off
rows = [[f'{html_bgcolor(connector.bgcolor_title)}{remove_links(connector.name)}'
if connector.show_name else None],
Expand All @@ -199,16 +205,23 @@ def create_graph(self) -> Graph:
html_line_breaks(pn_info_string(HEADER_SPN, connector.supplier, connector.spn))],
[html_line_breaks(connector.type),
html_line_breaks(connector.subtype),
f'{connector.pincount}-pin' if connector.show_pincount else None,
f'{connector.pincount}-pin' if connector.show_pincount else None],
[html_caption(connector.image) if connector.image and connector.image.position == 'above' else None,
html_image(connector.image) if connector.image and connector.image.position == 'above' else None,
translate_color(connector.color, self.options.color_mode) if connector.color else None,
html_colorbar(connector.color)],
Comment on lines +208 to 212
Copy link
Collaborator

@kvid kvid Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
f'{connector.pincount}-pin' if connector.show_pincount else None],
[html_caption(connector.image) if connector.image and connector.image.position == 'above' else None,
html_image(connector.image) if connector.image and connector.image.position == 'above' else None,
translate_color(connector.color, self.options.color_mode) if connector.color else None,
html_colorbar(connector.color)],
f'{connector.pincount}-pin' if connector.show_pincount else None,
translate_color(connector.color, self.options.color_mode) if connector.color else None,
html_colorbar(connector.color)],
*(image_rows if connector.image and connector.image.position == 'above' else []),

Bugs:

  • The row containing type, subtype, pincount, and color was broken into two rows (even if no above image present).
  • When above image present, it had caption to the left and color to the right - all 3 at the same row. I assume this was a mistake, and not intended.

Fix:

  • Reverse breaking the existing row.
  • Insert any above image rows just above the connector table.

'<!-- connector table -->' if connector.style != 'simple' else None,
[html_image(connector.image)],
[html_caption(connector.image)]]
'<!-- connector table -->' if connector.style != 'simple' else None]
# fmt: on
imagetable=''
Copy link
Collaborator

@kvid kvid Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
imagetable=''
image_table = ""
  • Suggest a name similar to other image variables
  • Black formatted

if connector.image:
if connector.image.position == 'below' or connector.image.position is None:
Copy link
Collaborator

@kvid kvid Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if connector.image.position == 'below' or connector.image.position is None:
if connector.image.position == "below":
  • Simplify because None is no longer allowed
  • Black formatted

rows += image_rows
else:
imagetable = ''.join(nested_html_table(image_rows, html_bgcolor_attr(connector.bgcolor)))
Copy link
Collaborator

@kvid kvid Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
imagetable = ''.join(nested_html_table(image_rows, html_bgcolor_attr(connector.bgcolor)))
image_table = "\n ".join(nested_html_table(image_rows)).replace(
' cellborder="1"', ""
)
  • Add linefeed and indent between rows for readable HTML
  • No need to apply bgcolor to inner table when outer table already has the same bgcolor
  • Remove cellborder to avoid double border
  • Black formatted


rows.extend(get_additional_component_table(self, connector))
rows.append([html_line_breaks(connector.notes)])

html.extend(nested_html_table(rows, html_bgcolor_attr(connector.bgcolor)))

if connector.style != "simple":
Expand All @@ -217,18 +230,28 @@ def create_graph(self) -> Graph:
'<table border="0" cellspacing="0" cellpadding="3" cellborder="1">'
)

for pinindex, (pinname, pinlabel, pincolor) in enumerate(
zip_longest(
firstpin = True
pincount = len(connector.pins)
if len(connector.pinlabels) > pincount:
pincount = len(connector.pinlabels)
if len(connector.pincolors) > pincount:
pincount = len(connector.pincolors)

Copy link
Collaborator

@kvid kvid Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if connector.hide_disconnected_pins:
pincount = sum(connector.visible_pins.values())

Bug:

  • rowspan becomes too large when hiding disconnected pins

Fix:

  • Count only visible pins in that case

for pinindex, (pinname, pinlabel, pincolor) in enumerate(zip_longest(
Copy link
Collaborator

@kvid kvid Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for pinindex, (pinname, pinlabel, pincolor) in enumerate(zip_longest(
for pinindex, (pinname, pinlabel, pincolor) in enumerate(
zip_longest(
  • Black formatting reversed your change

connector.pins, connector.pinlabels, connector.pincolors
)
):
)):
Copy link
Collaborator

@kvid kvid Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
)):
)
):
  • Black formatting reversed your change

if (
connector.hide_disconnected_pins
and not connector.visible_pins.get(pinname, False)
):
continue

pinhtml.append(" <tr>")

if firstpin and connector.image and connector.image.position == 'left':
firstpin = False
pinhtml.append(f'<td rowspan="{pincount}">{imagetable}</td>')

if connector.ports_left:
pinhtml.append(f' <td port="p{pinindex+1}l">{pinname}</td>')
Comment on lines +250 to 256
Copy link
Collaborator

@kvid kvid Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if firstpin and connector.image and connector.image.position == 'left':
firstpin = False
pinhtml.append(f'<td rowspan="{pincount}">{imagetable}</td>')
if connector.ports_left:
pinhtml.append(f' <td port="p{pinindex+1}l">{pinname}</td>')
if connector.ports_left:
pinhtml.append(f' <td port="p{pinindex+1}l">{pinname}</td>')
if (
firstpin
and connector.image
and connector.image.position == "left"
):
firstpin = False
pinhtml.append(
f' <td rowspan="{pincount}">{image_table}</td>'
)
  • Any left column with connected port cells MUST be the leftmost column for drawing the wire splines correctly
  • Add indent for readable HTML
  • Black formatted

if pinlabel:
Expand All @@ -248,6 +271,11 @@ def create_graph(self) -> Graph:

if connector.ports_right:
pinhtml.append(f' <td port="p{pinindex+1}r">{pinname}</td>')

if firstpin and connector.image and connector.image.position == 'right':
firstpin = False
pinhtml.append(
f'<td rowspan="{pincount}">{imagetable}</td>')
Comment on lines 272 to +278
Copy link
Collaborator

@kvid kvid Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if connector.ports_right:
pinhtml.append(f' <td port="p{pinindex+1}r">{pinname}</td>')
if firstpin and connector.image and connector.image.position == 'right':
firstpin = False
pinhtml.append(
f'<td rowspan="{pincount}">{imagetable}</td>')
if (
firstpin
and connector.image
and connector.image.position == "right"
):
firstpin = False
pinhtml.append(
f' <td rowspan="{pincount}">{image_table}</td>'
)
if connector.ports_right:
pinhtml.append(f' <td port="p{pinindex+1}r">{pinname}</td>')
  • Any right column with connected port cells MUST be the rightmost column for drawing the wire splines correctly
  • Add indent for readable HTML
  • Black formatted

pinhtml.append(" </tr>")

pinhtml.append(" </table>")
Expand Down Expand Up @@ -713,3 +741,4 @@ def bom(self):
if not self._bom:
self._bom = generate_bom(self)
return self._bom