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

Enable dd blocks by default and remove global setting #57036

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

mhugent
Copy link
Contributor

@mhugent mhugent commented Apr 4, 2024

Enable dd blocks by default and remove global setting 'enable-datadefined-blocks'. This PR replaces #56984

@github-actions github-actions bot added this to the 3.38.0 milestone Apr 4, 2024
Copy link

github-actions bot commented Apr 4, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 5aa718e)

@@ -149,7 +149,7 @@ QgsVectorLayerAndAttributeModel::QgsVectorLayerAndAttributeModel( QgsLayerTree *
const QgsVectorLayer *vLayer = qobject_cast< const QgsVectorLayer *>( QgsProject::instance()->mapLayer( id ) );
if ( vLayer )
{
mCreateDDBlockInfo[vLayer] = QgsDxfExportDialog::settingsDxfEnableDDBlocks->value();
mCreateDDBlockInfo[vLayer] = true;
Copy link
Member

Choose a reason for hiding this comment

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

It would be really handy to have a constant that can be used across all DXF Export classes to access this default value.

Just saying because I'm preparing another PR (see my local PR in the meantime), where I actually need the default value to improve code readability and maintenance (namely here to have something like if ( allowDataDefinedBlocks != DEFAULT_DXF_DATA_DEFINED_BLOCKS)).

Without such constant, it would be too easy for someone to break something in the future regarding this default value.

I guess it could live in the dxfexport.h or in the qgis.cpp.

Would that make sense for you @mhugent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sounds good to have it in dxfexport.h

@mhugent mhugent merged commit 6a1b775 into qgis:master Apr 10, 2024
29 checks passed
@gacarrillor
Copy link
Member

@mhugent, you had already implemented the DEFAULT_DXF_DATA_DEFINED_BLOCKS constant, hadn't you?
Did I miss something?

@mhugent
Copy link
Contributor Author

mhugent commented Apr 10, 2024

@mhugent, you had already implemented the DEFAULT_DXF_DATA_DEFINED_BLOCKS constant, hadn't you? Did I miss something?

I did. But you are right, I cannot find it in the PR anymore. Don't know if it got lost with one of the rebases

@mhugent
Copy link
Contributor Author

mhugent commented Apr 10, 2024

@gacarrillor : I'll open a new PR for it. Do you want to first merge your other PR?

@gacarrillor
Copy link
Member

@gacarrillor : I'll open a new PR for it. Do you want to first merge your other PR?

If you mean #57016, yes, I'm currently rebasing (solving conflicts) with the changes you introduced in this PR and some from #57103, and hopefully it will soon be ready to merge.

Once it's merged, I'll bring this one (which is currently against my own master) to the official QGIS repo. For that one it would be great to have the default constant implemented. To make things easier, I could ping you once it's been created against QGIS master and I'd appreciate if you could commit to it bringing the default constant.

Sounds good to you?

@mhugent
Copy link
Contributor Author

mhugent commented Apr 10, 2024

Sounds good to you?

+1, waiting for your ping.

I found out why it got lost. Implemented it on laptop A, then did the rebase from laptop B using force-push and without pulling first...

@gacarrillor
Copy link
Member

I found out why it got lost. Implemented it on laptop A, then did the rebase from laptop B using force-push and without pulling first...

Ouch!

+1, waiting for your ping.

Thank you @mhugent, I'll let you know!

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