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

ketanhwr
Copy link
Contributor

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

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

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


setSelectedHandles(QSet<PointHandle*>());

mapDocument()->mapObjectModel()->setObjectPolygon(mapObject, newPolygon);
Copy link
Member

Choose a reason for hiding this comment

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

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

@bjorn
Copy link
Member

bjorn 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
Copy link
Contributor Author

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
Copy link
Member

bjorn 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 mapeditor:master Aug 15, 2017
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.

None yet

2 participants