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

Draft: Add "legacy importer only" to DXF option #13638

Merged

Conversation

Roy-043
Copy link
Contributor

@Roy-043 Roy-043 commented Apr 25, 2024

Fixes #13598.

@Roy-043 Roy-043 added the WB Draft Related to the Draft Workbench label Apr 25, 2024
@furgo16
Copy link
Contributor

furgo16 commented Apr 25, 2024

@Roy-043 thanks! I think there are stilll more capabilities in the preferences dialog that are legacy-importer-only. I wonder if there is a more concise way to mark them without appending the ("legacy importer only") string to them.

I did a side by side comparison table here: #13599 (comment)

Here's a mockup of what I had in mind:

image

Conceivably, a better implementation would be to disable the relevant options when the Use legacy python exporter checkbox is ticked. However, as a first iteration, just marking those options would already go a long way preventing user confusion and spending time in trial-and-error importer passes. Those can take a quite a long time depending on the imported file's complexity, and the options chosen.

@Roy-043
Copy link
Contributor Author

Roy-043 commented Apr 26, 2024

Thanks for the detailed analysis. It is indeed better to do this properly. I'll convert this PR to Draft for now. If I remember correctly there may even be a setting that affects both import and export...

@Roy-043 Roy-043 marked this pull request as draft April 26, 2024 08:41
@furgo16
Copy link
Contributor

furgo16 commented Apr 26, 2024

Thank you. I just finished the analysis and updated the original comment. The comparison table is now up-to-date to the best of my knowledge.

@furgo16
Copy link
Contributor

furgo16 commented May 1, 2024

Related: #13751

Legacy only items are en/disabled depending on the selected legacy options. The "Some options are not yet available for ..." texts are in italics.
@Roy-043 Roy-043 marked this pull request as ready for review May 3, 2024 16:41
@Roy-043
Copy link
Contributor Author

Roy-043 commented May 3, 2024

Result of the updated PR:
DXF-options

@furgo16
Copy link
Contributor

furgo16 commented May 3, 2024

This is excellent, thanks so much. This removes the previous uncertainty, it will make life easier and it will prevent unnecessary trial and error trying combinations of unimplemented options. Some other comments:

  1. Is it worth considering sensible default settings in this PR, or rather discuss it in a separate issue? If so, happy to file it. Otherwise, in addition to the options already ticked in the screenshot, I'd suggest to tick the following ones as well:
    • Show this dialog when importing and exporting => once there is one single importer with feature parity with the old one, and it is reasonably stable, this setting could be unticked as default for a more seamless import/open operation. But I think for now, it's worth letting the user know what and how will be imported, and showing them how to control that.
    • texts and dimensions => I could imagine that given the low input in importing and processing them, and that they seem to be reasonably well supported, most folks would like to keep texts and dimensions from the original DXF file.
    • Use colors from the DXF file => same as above
  2. I'm not familiar enough with the exporter part to be able to comment on that section.
  3. I know it's not part of this PR, but since it's a safe one-liner, and it's less overhead to change it here than on a separate PR: Use Layers => Use layers. Change to sentence case to be consistent with the rest of the copy on the dialog. Particularly "Group layers into blocks"

@furgo16
Copy link
Contributor

furgo16 commented May 8, 2024

Does this PR need further testing or review before getting merged? Thanks.

@Roy-043
Copy link
Contributor Author

Roy-043 commented May 9, 2024

The Show this dialog when importing and exporting option is now checked by default. Changing the other preferences does not necessarily make sense for other workflows where a DXF is just used as a vector format, for example to import the contour of a product.

@Roy-043 Roy-043 merged commit 2f9b97c into FreeCAD:main May 9, 2024
9 checks passed
@Roy-043 Roy-043 deleted the Draft-Add-legacy-importer-only-to-DXF-option branch May 9, 2024 09:12
@furgo16
Copy link
Contributor

furgo16 commented May 9, 2024

Makes sense, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB Draft Related to the Draft Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DXF Importer (C++) ignores the "Create as" import settings and always creates simple Part shapes
2 participants