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

Cura 11561 mockup pap #18246

Merged
merged 21 commits into from
Feb 23, 2024
Merged

Cura 11561 mockup pap #18246

merged 21 commits into from
Feb 23, 2024

Conversation

wawanbreton
Copy link
Contributor

@wawanbreton wawanbreton commented Feb 5, 2024

Implements a mockup for testing the Printer Agnostic Project new feature. This is really a preview and would probably require some technical+feature polishing to be fully usable.

Don't merge to main !

CURA-11561

@@ -228,11 +234,14 @@ def preRead(self, file_name, show_dialog=True, *args, **kwargs):
self._resolve_strategies = {k: None for k in resolve_strategy_keys}
containers_found_dict = {k: False for k in resolve_strategy_keys}

# Check whether the file is a UCP, which changes some import options
is_ucp = file_name.endswith('.ucp')
Copy link
Member

Choose a reason for hiding this comment

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

It's fine for now, but for the future i would just look for the extra settings file. That way it's already compatible if we ever decide to merge it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be cleaner indeed 👍


@call_on_qt_thread
def write(self, stream, nodes, mode=WorkspaceWriter.OutputMode.BinaryMode):
def _preWrite(self):
Copy link
Member

Choose a reason for hiding this comment

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

Not the greatest solution, but it's fine for the mockup. When integrating this, i do think we should split off the preWrite.

Copy link
Member

Choose a reason for hiding this comment

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

Also, why not also just give the pre_write an extra param if it's an UCP or not. You can use a lambda in the call later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reader classes have explicit preRead and read methods, so we should probably do the same for the writers. I didn't want to go there for the mockup, but for integration yes.

For the extra param, I don't really understand what you mean. What I think is that we should add a param to the write method which is the mimetype of the file to be written. Or allow the mode param (which is currently useful only for the STLs) to be any type, and then have a specific enum for the 3MF/UCP.

@saumyaj3 saumyaj3 marked this pull request as ready for review February 16, 2024 10:49
@saumyaj3 saumyaj3 marked this pull request as draft February 16, 2024 10:49
@casperlamboo casperlamboo merged commit 80d7536 into main Feb 23, 2024
10 checks passed
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.

4 participants