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

Make better use of part names #644

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

Conversation

chrysn
Copy link
Contributor

@chrysn chrysn commented Mar 5, 2024

So far, part names are only used to place red text labels.

This change stores the names in the parts (the constructor argument was previously ignored; an extra argument is added to new_part to account for its typical usage pattern where drawing happens on the last set part, and a new part is created at the end of drawing).

A second commit uses the data that is now stored to make names accessible in browsers' tooltips. Future changes enabled by this would be making parts more easily accessible in DXF as well, eg. to extract them for further processing into 3D models.

In web browsers, this shows tooltips when the cursor hovers over lines
of a part.
@chrysn chrysn mentioned this pull request Mar 11, 2024
@florianfesti
Copy link
Owner

I very much like what this is aiming at. I am not that much a fan of the implementation. Let's just make sure we pass the label to the first call of move(), too. add_part might need a bit of tweaking to make sure we don't keep the old empty part with the old label. But that should just be a single line.

@chrysn
Copy link
Contributor Author

chrysn commented Mar 23, 2024

Oh I fully agree that the retronaming is not pretty. I've tried to make all changes such that no generators would be changed, given that boxes.py can also be used as a library and I don't know what the API guarantees there are.

If I start an alternative version of this where there is a mandatory new_part called at the start of a part rather than at the end (which also makes later steps easier), is there a test suite somewhere that I can run that will execute all generators at least with default configuration? (That'd be helpful because I could make it scream the right way until every move start matches its move end). Ideally, is there something that I can run that asserts that the box outputs did not change?

@chrysn
Copy link
Contributor Author

chrysn commented Mar 23, 2024

And because I'll have to deal with the temptation: Is there a rationale for why the move blocks are not implemented using context managers? (The refactoring necessary to do the labels in the first part will note quite warrant that kind of change, but the temptation will be there).

@florianfesti
Copy link
Owner

I think the main reason is that context managers did not exist in Python when the code was written. Also passing width and height is super annoying. This was necessary in the past as Boxes.py used libcairo. As we now use our own backend, we should be able to calculate the space needed by the pieces and place them correctly accordingly.

@florianfesti
Copy link
Owner

Wait a second. Part now has both .extents and .transform. This might be easier than I thought. Give me a minute...

@chrysn
Copy link
Contributor Author

chrysn commented Apr 5, 2024

Would you care to give me some more pointers as to what you'd like to have simplified? (I can also start refactoring the label assignments based on giving the part name beforehand, but it seems you have more concrete ideas.)

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.

2 participants