Goto Point on Map Click

[ericjohnson97]

This merge request adds the ability to command to goto a point by clicking on the map widget.

[patrickelectric]

Hi Eric!

Thanks for your contributions!
We squashed the previous PRs to be merged, but for now on I would recommend to improve a bit the commit history of your PRs.
As you can see, we try to break the changes on atomic commits while avoiding development commits of the PR to be merged as well.
“Merge” commits are also something to avoid, I highly recommend using git rebase origin/master to rebase your work, not merge.
Now, to improve your commit history, I highly recommend something closer to the following actions:

git reset HEAD~9 # this is the number of commits that you have on your branch
git add -p # Add only the necessary parts for a specific commit
git commit (-sm message ”if you want to avoid the editor”) # Create your commit
# Repeat step two until it’s done

In this case, could be something like:
One commit for the Map widget
One commit for Ardupilot goto command
One commit for the mainVehicle

And your PR history will be clean to merge on your master branch :)

If you have any questions don't hesitate to ask, we can help you out with it.

[rafaellehmkuhl]

Just minors and the context-menu fix from my side.

The PR looks great, it just needs to be rebased as @patrickelectric mentioned.

[ericjohnson97]

Hi Eric!

Thanks for your contributions! We squashed the previous PRs to be merged, but for now on I would recommend to improve a bit the commit history of your PRs. As you can see, we try to break the changes on atomic commits while avoiding development commits of the PR to be merged as well. “Merge” commits are also something to avoid, I highly recommend using git rebase origin/master to rebase your work, not merge. Now, to improve your commit history, I highly recommend something closer to the following actions:

git reset HEAD~9 # this is the number of commits that you have on your branch
git add -p # Add only the necessary parts for a specific commit
git commit (-sm message ”if you want to avoid the editor”) # Create your commit
# Repeat step two until it’s done

In this case, could be something like: One commit for the Map widget One commit for Ardupilot goto command One commit for the mainVehicle

And your PR history will be clean to merge on your master branch :)

If you have any questions don't hesitate to ask, we can help you out with it.

I just did what you suggested.

I haven't done development like this in the past. Is this done to minimize commits in the repo? Is your usual workflow have dev commits and then rewrite the history or do you usually just leave thing un committed until you know a feature is ready?

Just trying to figure out the core of the reasoning and the best work flow for the future

[ES-Alexander]

Hi @ericjohnson97, just jumping in here on the "reasoning" front:

I haven't done development like this in the past. Is this done to minimize commits in the repo? Is your usual workflow have dev commits and then rewrite the history or do you usually just leave thing un committed until you know a feature is ready?

The development history of a project is an important tool for understanding when, where, and why a particular change was made, and keeping the commit history "clean" helps significantly to that end. Rewriting history that multiple people have access to is problematic, because it's hard to develop over ground that's moving beneath your feet, so once a commit is merged into one of our organisation repositories it gets kept as-is.

Accordingly,

1. it's in everyone's best interests to have all the public/official commits be meaningful, with descriptive messages as to their purpose, but
2. it's irrelevant whether individuals working on local forks/copies actually write code directly like that, or write several dev commits that then get rebased and cleaned up into the "presentation" history that gets reviewed in a pull request

We generally start out with frequent small dev commits, that then get reordered and cleaned up before the PR gets reviewed, because it's easier to merge commits together than split them apart. That said, knowing the end goal does help encourage at least vaguely relevant commit messages during development, and for large changes encourages cleaning up the history from time to time as you go.

Just trying to figure out the core of the reasoning and the best work flow for the future

I would recommend developing with small commits, then if you want people to test the functionality before you clean up your code you can open a Draft Pull Request (so people can find it easily, and so relevant CI processes/checks can run), and then once the code is cleaned up you can click the "Ready for Review" button to turn it into a mergeable PR.

We do also have some general version control guidelines (relevant to all our repositories), although they haven't been updated in a while so there may be some recent changes or technologies that have caused slight differences in how we actually operate. If you've got more development process questions, feel free to ask them in discord or the Blue Robotics forums, or by raising issues in our software guidelines repo :-)

[ES-Alexander]

Just to clarify, the purpose is not necessarily to have one commit per file, but sometimes that does work out to be what's most reasonable. The point is that each commit should be a meaningful (generally self-contained) change.

Ideally it's possible to pull out one commit without majorly affecting anything else (beyond features that build off it, of course), and similarly it's ideal if the code is still buildable/runnable if the history is reverted to directly after any arbitrary commit. Since goTo in the map widget is UI-focused, and requires the functionality to be defined before it's usable, the map commit should likely come last.

For some extra context, the git commit history doesn't know anything about pull requests (only GitHub knows about that), so commits and their messages should preferably be meaningful within the context of the whole repository, not just within a given pull request. In that vein we'll typically start messages with which part of the codebase the commit is focused on, or at least which features it's related to (e.g. widgets: map: add goto functionality).

[ericjohnson97]

@ES-Alexander Thanks for the explanation. I will try to clean up my commit history before pull request from now on and follow the software guidelines.

@rafaellehmkuhl are there any other things that need to get resolved?

[rafaellehmkuhl]

@ES-Alexander Thanks for the explanation. I will try to clean up my commit history before pull request from now on and follow the software guidelines.

@rafaellehmkuhl are there any other things that need to get resolved?

The code is already good! I would only change the commit messages to adhere to our committing style. As @ES-Alexander mentioned, we try to mention the sector being changed, followed by the change being made, with the change phrase starting capitalized and the two separated by a colon.

May I suggest the following commit messages? This way we can skip this step for now and already merge everything.

• map-widget: Allow vehicle reposition (goTo) on click
• ardupilot: Support "goTo" functionality with MAV_CMD_DO_REPOSITION commands
• main-vehicle: Add goTo functionality

[rafaellehmkuhl]

It looks good to me. @ericjohnson97 can we merge?

[ericjohnson97]

@rafaellehmkuhl I'm ready 🙂