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

Add option to add colors to connector pins #141

Merged
merged 4 commits into from
Nov 14, 2020
Merged

Conversation

formatc1702
Copy link
Collaborator

@formatc1702 formatc1702 commented Jul 30, 2020

Closes #53.
Based on feature/gv-html-refactor branch.

  • Adds new pincolors list attribute to connectors.
    • If pincolors is used, needs to be the same length as pins, pinlabels
  • If used, displays a small color mark and label next to each pin
    • Color mark is smaller than the connector.color mark on purpose
  • Implements __ as placeholder for pins with no color
    • Better suggestions welcome
    • Edit 2020-10-21: Any unknown color code is treated as "no color", so __, --, XX and anything else may be used.
  • Makes sure white color marks look decent on white background
    • TODO in the future: detect what background color is used, and react accordingly

Example:

connectors:
  X1:
    type: D-Sub
    subtype: female
    color: GY
    pins:      [5,   2,  3,  99,  98,  97,  96,  95,  94]
    pinlabels: [DCD, RX, TX, DTR, GND, DSR, RTS, CTS, RI]
    pincolors: [PK,  RD,  OG, YE, GN,  TQ,  LB,  BU,  VT]
  X2:
    type: Molex KK 254
    subtype: female
    color: BG
    pinlabels: [GND, RX, TX, N/C, N/C, N/C]
    pincolors: [GD,  SR, CU, __ , BK,  WH ]

cables:
  W1:
    gauge: 0.25 mm2
    length: 0.2
    color_code: DIN
    wirecount: 3
    shield: true

connections:
  -
    - X1: [5,2,3]
    - W1: [1,2,3]
    - X2: [1,3,2]
  -
    - X1: 5
    - W1: s

test

@formatc1702 formatc1702 force-pushed the feature/pincolors branch 3 times, most recently from 825ea4d to 127c720 Compare July 30, 2020 17:57
@formatc1702
Copy link
Collaborator Author

formatc1702 commented Jul 30, 2020

Using circles instead of squares and no division between color label and color mark, maybe a bit nicer

test

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.

I'm sorry for picking on all these small issues, but when reviewing, I want to do a thorough job, and include all issues I feel can be improved, but I respect that others might disagree on my issues.

src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/DataClasses.py Outdated Show resolved Hide resolved
src/wireviz/wv_colors.py Outdated Show resolved Hide resolved
src/wireviz/wv_colors.py Outdated Show resolved Hide resolved
@formatc1702
Copy link
Collaborator Author

I'm sorry for picking on all these small issues, but when reviewing, I want to do a thorough job, and include all issues I feel can be improved, but I respect that others might disagree on my issues.

Your nitpicking is excellent and much appreciated. The point of doing code review is to produce cleaner, easier to understand code. I will include most of your suggestions and reply to them if anything is unclear.
Thanks!

@kvid

This comment has been minimized.

@formatc1702

This comment has been minimized.

@kvid

This comment has been minimized.

@formatc1702 formatc1702 added this to the v0.3 or later milestone Aug 2, 2020
Base automatically changed from feature/gv-html-refactor to dev August 13, 2020 15:23
@formatc1702
Copy link
Collaborator Author

Playing around a bit more.

The current output generates a small black outline around each pin color box. 🎉

test

If the connector node is stretched due to long parameter strings in the top row, the pin color cells end up looking rather wide, since GraphViz autoscales all columns. The problem is worsened because two cells/columns are needed for the pin color: one for the color string, one for the little box. GraphViz does not allow mixing strings and nested tables within a single cell. Using fixedsize and width attributes on the cell reduces the actual cell width (visualized below using bgcolor), but does not auto-increase all the other cells' widths to compensate, which is sad.

test

@SnowMB
Copy link
Contributor

SnowMB commented Oct 21, 2020

I played with this a while ago. The problem is, that there is no way mixing fixed sized and auto-sized colums so all colums are stretched evenly. Making the content of a cell fixed size (like your 2nd image) does not help at all. I have not found a way around that so eventually I gave up.

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Oct 21, 2020

TBH, I'm quite happy with the result so far (first picture), and wouldn't mind leaving it like it is.
The whole pin colors thing seems like a niche application to me, so I wouldn't want to invest too much time into it right now.
If we get feedback that people use this feature, and want improvements, we can react later.
I can probably rebase onto dev and clean up the code in the upcoming days.

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Oct 23, 2020

Feature has been rebased onto dev and is ready for review and merging.

Sample output

image

I'm sure the appearance / column width can be further optimized; however, unless there is a quick fix that can be implemented now, I'd rather close this PR and wait for user feedback.

src/wireviz/Harness.py Outdated Show resolved Hide resolved
@formatc1702
Copy link
Collaborator Author

@kvid I tried to request a review from you, but it seems that isn't working. So please have a look at this and let me know your thoughts :) As mentioned above, I'm sure the appearance can be fine-tuned, but I want to get the feature included to get some feedback.

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.

By using itertools.zip_longest as you have already considered, you can not only simplify your new code, but also some of the old Connector.__post_init__() code.

I also miss a demonstration of this feature in one of the examples.

src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/DataClasses.py Show resolved Hide resolved
src/wireviz/DataClasses.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
@formatc1702
Copy link
Collaborator Author

Thanks. I have implemented your suggestions; I appreciate the simplification of the code.

Regarding examples: I will need to add/change a lot of examples once #186 is implemented, therefore I will wait until then. I hope to create some more realistic examples to showcase the individual features better, not just randomly add it to one of the current ones.

Therefore, I will go ahead and merge this soon.

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 my previous suggestions.
This time, I only have a few not-so-important suggestions...

src/wireviz/DataClasses.py Outdated Show resolved Hide resolved
src/wireviz/DataClasses.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
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 most of my suggestions again. As you want to keep the non-zero pincount requirement, then I suggest simplifying this test and also allow pincolors list length to set default pincount. Then all the 3 pin attribute lists influence the pincount equally.

src/wireviz/DataClasses.py Outdated Show resolved Hide resolved
Co-authored-by: kvid <kvid@users.noreply.github.com>
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 my last suggestion as well. I think this PR is ready now. 😃

@formatc1702 formatc1702 merged commit feff47f into dev Nov 14, 2020
@formatc1702 formatc1702 deleted the feature/pincolors branch November 14, 2020 08:43
formatc1702 added a commit that referenced this pull request Nov 14, 2020
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.

3 participants