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 the possibility to rotate a single feature (line or polygon) #5737

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

Conversation

qsavoye
Copy link

@qsavoye qsavoye commented Oct 15, 2024

add_rotate_feature

@qsavoye
Copy link
Author

qsavoye commented Oct 17, 2024

I just remove some submodules depency that I do not had to commit previously, sorry for the incovenient.

@nirvn
Copy link
Member

nirvn commented Oct 25, 2024

@qsavoye , hey there, thanks for this contribution, it's fantastic! I'll get to review it ASAP (I've been a bit under the weather this week).

One thing I can already see as a need here is to change the icon to use a stock one from Google's material symbols & icons library: https://fonts.google.com/icons?icon.query=rotate -- that'll insure the style matches the rest of the app.

@nirvn
Copy link
Member

nirvn commented Oct 25, 2024

@qsavoye , oh, one question: you are aware that the processing toolbox (new feature in QField 3.4) has a rotate algorithm. Is there a reason why you wouldn't be using that instead?

@qsavoye
Copy link
Author

qsavoye commented Oct 25, 2024

@qsavoye , oh, one question: you are aware that the processing toolbox (new feature in QField 3.4) has a rotate algorithm. Is there a reason why you wouldn't be using that instead?

@nirvn It's more a question of user experience. I tried the processing toolbox for rotation on my Iphone, the parameter window hides a big part of the map canvas, which can be unpleasant
Furthermore, in my opinion, it is faster to rotate with drag or mouse pointer on mapcanvas than with a precise angle in degrees. I think both are complementary.

Copy link
Member

@nirvn nirvn left a comment

Choose a reason for hiding this comment

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

@qsavoye , hey there, sorry for the delayed review, it's been one of those months :)

This would make a great feature for QField's next release (3.5). Let me know if you have questions on the review comments.

i18n/qfield_en.ts Outdated Show resolved Hide resolved
src/core/featurelistextentcontroller.cpp Outdated Show resolved Hide resolved
src/core/featurelistextentcontroller.cpp Outdated Show resolved Hide resolved
src/core/featurelistextentcontroller.cpp Outdated Show resolved Hide resolved
src/core/featurelistextentcontroller.cpp Outdated Show resolved Hide resolved
src/qml/MapCanvas.qml Outdated Show resolved Hide resolved
src/qml/NavigationBar.qml Outdated Show resolved Hide resolved
src/qml/qgismobileapp.qml Outdated Show resolved Hide resolved
src/qml/qgismobileapp.qml Outdated Show resolved Hide resolved
src/qml/qgismobileapp.qml Outdated Show resolved Hide resolved
@nirvn
Copy link
Member

nirvn commented Nov 19, 2024

@qsavoye , thanks for addressing the review. Will merge as soon as the CIs turn green. Thanks for this contribution 🙏

@qsavoye
Copy link
Author

qsavoye commented Nov 19, 2024

@nirvn I think my there was an update between the current master and and the master where i start these pull request on file multifeaturelistmodelbase.cpp

in canRotateSelection function

if ( !vlayer || vlayer->readOnly() || !( vlayer->dataProvider()->capabilities() & QgsVectorDataProvider::ChangeGeometries ) || vlayer->customProperty( QStringLiteral( "QFieldSync/is_geometry_locked" ), false ).toBool() )

must be replace to

if ( !vlayer || vlayer->readOnly() || !( vlayer->dataProvider()->capabilities() & Qgis::VectorProviderCapability::ChangeGeometries ) || vlayer->customProperty( QStringLiteral( "QFieldSync/is_geometry_locked" ), false ).toBool() )

Do you see other change which can block the merge ?

@nirvn
Copy link
Member

nirvn commented Nov 19, 2024

@qsavoye , no, if that's fixed, I think we can merge.

There was indeed a QGIS core library update which did change a couple of enum keys, inc. this one.

@nirvn
Copy link
Member

nirvn commented Nov 19, 2024

@qsavoye , can you resolve the conflict too?

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