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

Adds option to remove an edge from polygon #1685

Merged
merged 6 commits into from Aug 15, 2017

Conversation

Projects
None yet
2 participants
@ketanhwr
Copy link
Contributor

commented Aug 13, 2017

If two consecutive nodes are selected, then you can delete the edge between them to convert a polygon to polyline.

ketanhwr added some commits Aug 13, 2017

@bjorn
Copy link
Owner

left a comment

A few points of feedback on the code.

@@ -618,6 +620,24 @@ void EditPolygonTool::showHandleContextMenu(PointHandle *clickedHandle,
connect(joinNodesAction, SIGNAL(triggered()), SLOT(joinNodes()));
connect(splitSegmentsAction, SIGNAL(triggered()), SLOT(splitSegments()));

const auto firstHandle = *mSelectedHandles.begin();
const auto secondHandle = *(mSelectedHandles.begin() + 1);

This comment has been minimized.

Copy link
@bjorn

bjorn Aug 15, 2017

Owner

Please get the second handle only after checking if there is one.

bool enabled = false;
if (mSelectedHandles.size() == 2) {
int indexDifference = std::abs(firstHandle->pointIndex() - secondHandle->pointIndex());
if (indexDifference == 1 || indexDifference == mapObject->polygon().size() - 1)

This comment has been minimized.

Copy link
@bjorn

bjorn Aug 15, 2017

Owner

Don't forget to also check that both points belong to the same object.


setSelectedHandles(QSet<PointHandle*>());

mapDocument()->mapObjectModel()->setObjectPolygon(mapObject, newPolygon);

This comment has been minimized.

Copy link
@bjorn

bjorn Aug 15, 2017

Owner

Instead of setting it here, you could use the ChangePolygon constructor that takes both the old and the new polygon.

ketanhwr and others added some commits Aug 15, 2017

@bjorn

This comment has been minimized.

Copy link
Owner

commented Aug 15, 2017

I pushed a small change that was more in the direction where I was trying to hint in my last review.

In general, I think users will wonder:

  • Why this action does not work for splitting polylines (of course, that would create multiple objects, but why not)
  • Why it doesn't work on possibly multiple selected edges, like the "Split Segments" action
  • And just now, I realized we now have two terms for the same thing "edge" and "segment". I guess this action should be called "Delete Segment".
@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2017

Why it doesn't work on possibly multiple selected edges, like the "Split Segments" action.

The first and second point mainly refer to the same point. Personally, I don't think such an action would be common.

And just now, I realized we now have two terms for the same thing "edge" and "segment". I guess this action should be called "Delete Segment".

Right, I'll rename it.

@bjorn

This comment has been minimized.

Copy link
Owner

commented Aug 15, 2017

The first and second point mainly refer to the same point. Personally, I don't think such an action would be common.

I think it's quite normal to spend most of the time working on stuff that will be rarely used, but let's have this action as-is then and spend time on other things then. :-)

@bjorn bjorn merged commit ae09f0f into bjorn:master Aug 15, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.