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

[WIP]Add connector image alignment #420

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

Conversation

martonmiklos
Copy link
Contributor

Added a new 'position' property to connector images which can be used to position images above/right/left of the connector tables:

Above:
kép
Left:
kép
Right:
kép

- Rename image alignment to position
- Rename under to below
- Display the above positioned images right above the pins list

Fixes wireviz#418
@tobiasfalk
Copy link

The left and right option need also to be considered in #404😊

Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

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

Thank you for accepting and implementing my preliminary suggestions in #418 (comment) - and here follows my promised detailed comments/suggestions to your code changes:

  • Remember that most HTML generation code changes must later be adapted to Large scale refactoring #251 changes before a merge-in
  • Fixing a few bugs (see details in each comment)
  • Minor simplifications
  • Black formatting (remember running isort *.py and black *.py before committing your changes)
  • Please add an entry to docs/syntax.md that lists the alternative attribute values and briefly explains that they relate to the pin list of connectors.
  • Left/right images for simple connectors are still not implemented, and I suggest for now to only allow image.position==below for simple connectors and all cables, and raise ValueError otherwise in Connector.__post_init__() and Cable.__post_init__()

Github only allow me in this review to add code suggestions close to your code changes, but as an example to my latter point above, this could be added at the bottom of the existing if self.style == "simple": statement body in Connector.__post_init__():

            if self.image and self.image.position != "below":
                raise ValueError(
                    f"{self.name}.image.position='{self.image.position}', but simple connectors (with no pin table) only support the default 'below'"
                )

@@ -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

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.

Comment on lines +194 to +199

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

Copy link
Collaborator

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: on
imagetable=''
Copy link
Collaborator

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

Comment on lines +208 to 212
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)],
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
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.

# fmt: on
imagetable=''
if connector.image:
if connector.image.position == 'below' or connector.image.position is None:
Copy link
Collaborator

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

pincount = len(connector.pinlabels)
if len(connector.pincolors) > pincount:
pincount = len(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
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

if len(connector.pincolors) > pincount:
pincount = len(connector.pincolors)

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

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

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

Comment on lines +250 to 256

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>')
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 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

Comment on lines 272 to +278
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>')
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.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

kvid

This comment was marked as duplicate.

@kvid kvid linked an issue Sep 12, 2024 that may be closed by this pull request
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 this pull request may close these issues.

Alignment of the connector images
3 participants