[WIP]Add connector image alignment #420

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

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

src/wireviz/DataClasses.py

@@ -86,6 +87,7 @@ class Image:
    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

src/wireviz/Harness.py

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

src/wireviz/Harness.py

            # fmt: on
+            imagetable=''


Suggested change

imagetable=''

image_table = ""

Suggest a name similar to other image variables

Black formatted

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

src/wireviz/Harness.py

+                     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

src/wireviz/Harness.py

            # 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

src/wireviz/Harness.py

+                    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 some pins

Fix:

Count only visible pins in that case

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

src/wireviz/Harness.py

+                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

src/wireviz/Harness.py

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


Suggested change

)):

)

):

Black formatting reversed your change

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

src/wireviz/Harness.py

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

src/wireviz/Harness.py

                    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]Add connector image alignment #420

[WIP]Add connector image alignment #420

martonmiklos commented Sep 7, 2024

tobiasfalk commented Sep 7, 2024

kvid left a comment •

edited

Loading

kvid Sep 10, 2024 •

edited

Loading

kvid Sep 11, 2024

kvid Sep 11, 2024

kvid Sep 11, 2024 •

edited

Loading

kvid Sep 11, 2024

kvid Sep 11, 2024 •

edited

Loading

kvid Sep 11, 2024

kvid Sep 11, 2024

kvid Sep 11, 2024 •

edited

Loading

kvid Sep 11, 2024 •

edited

Loading

This comment was marked as duplicate.

	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(

[WIP]Add connector image alignment #420

Are you sure you want to change the base?

[WIP]Add connector image alignment #420

Conversation

martonmiklos commented Sep 7, 2024

tobiasfalk commented Sep 7, 2024

kvid left a comment • edited Loading

Choose a reason for hiding this comment

kvid Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

kvid Sep 11, 2024

Choose a reason for hiding this comment

kvid Sep 11, 2024

Choose a reason for hiding this comment

kvid Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

kvid Sep 11, 2024

Choose a reason for hiding this comment

kvid Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

kvid Sep 11, 2024

Choose a reason for hiding this comment

kvid Sep 11, 2024

Choose a reason for hiding this comment

kvid Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

kvid Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as duplicate.

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