[WIP]Add connector image alignment by martonmiklos · Pull Request #420 · wireviz/WireViz

martonmiklos · 2024-09-07T18:34:51Z

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

Above:

Left:

Right:

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

tobiasfalk · 2024-09-07T20:24:44Z

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

kvid

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

kvid · 2024-09-10T21:54:39Z

    height: Optional[int] = None
    fixedsize: Optional[bool] = None
    bgcolor: Optional[Color] = None
+    position: Optional[ImagePosition] = None


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.

kvid · 2024-09-11T00:00:14Z

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


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

kvid · 2024-09-11T00:24:12Z

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


Suggested change

imagetable=''

image_table = ""

Suggest a name similar to other image variables

Black formatted

kvid · 2024-09-11T00:37:52Z

+                     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)],


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.

kvid · 2024-09-11T00:38:24Z

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


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

kvid · 2024-09-11T00:45:29Z

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


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

kvid · 2024-09-11T00:50:08Z

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


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

kvid · 2024-09-11T00:51:28Z

                        connector.pins, connector.pinlabels, connector.pincolors
-                    )
-                ):
+                    )):


Suggested change

)):

)

):

Black formatting reversed your change

kvid · 2024-09-11T01:03:48Z

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


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

kvid · 2024-09-11T01:10:04Z

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


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

martonmiklos added 2 commits August 19, 2024 22:16

Added alignment option to the connector images

00716d0

Fix @kvid's suggestions:

f19fafb

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

martonmiklos force-pushed the add_connector_image_alignment branch from 5a273fc to f19fafb Compare September 7, 2024 18:35

martonmiklos mentioned this pull request Sep 7, 2024

Alignment of the connector images #418

Open

kvid requested changes Sep 11, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

kvid linked an issue Sep 12, 2024 that may be closed by this pull request

Alignment of the connector images #418

Open

kvid mentioned this pull request Oct 26, 2024

Accept different YAML types for image(s) #416

Draft

8 tasks

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

	if connector.image.position == 'below' or connector.image.position is None:
	if connector.image.position == "below":


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

	for pinindex, (pinname, pinlabel, pincolor) in enumerate(zip_longest(
	for pinindex, (pinname, pinlabel, pincolor) in enumerate(
	zip_longest(

Conversation

martonmiklos commented Sep 7, 2024

Uh oh!

tobiasfalk commented Sep 7, 2024

Uh oh!

kvid left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvid Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvid Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

kvid Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

kvid Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvid Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

kvid Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvid Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

kvid Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

kvid Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvid Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kvid left a comment •

edited

Loading

kvid Sep 10, 2024 •

edited

Loading

kvid Sep 11, 2024 •

edited

Loading

kvid Sep 11, 2024 •

edited

Loading

kvid Sep 11, 2024 •

edited

Loading

kvid Sep 11, 2024 •

edited

Loading