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
Feature: Contextual actions for vehicles grouped by shared orders #8425
Conversation
I often group vehicles by shared orders and would find this useful. If you rebase and fix it (the preview build failed), I promise to review it. 😄 |
Oh wow a two-year response time ;) I'll try to rebase it later tonight. |
I wasn't a developer two years ago. 🙂 But now I am and I'm trying to review the low-hanging fruit PRs that are within my abilities. |
@2TallTyler Done rebasing. |
There were some conflicts and the CI checks didn't run so I rebased. I'll try to review tomorrow. |
Sorry, the earlier rebase didn't update it much (I mistakenly rebased on origin/master instead of upstream/master). I've rebased it properly this time. |
41ca908
to
1e580e9
Compare
1e580e9
to
6b62e91
Compare
Thanks for the review! |
6b62e91
to
110eab6
Compare
110eab6
to
d98547b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as described. Thanks for the detailed test instructions!
Code looks fine to me, but should definitely get a second pass before merging.
Motivation
This is a possible addition to #7028 to allow (some) contextual actions when vehicle lists are grouped by shared orders. Contextual actions are clicks on a vehicle in the vehicle list that complete some other ongoing action, such as to clone a vehicle or to copy/share orders.
This brings the option with grouping enabled to "parity" with the non-grouped option, by supporting contextual actions wherever it is unambiguous.
Description
The main addition of this PR is the contextual callback (
VehicleClicked()
) overload for multiple vehicles. This PR is implemented as a generic mechanism for contextual actions on sets containing multiple vehicles, instead of just a single vehicle. It hence does not rely on anything specific to grouping by shared orders, so it will still work even if in the future we have other ways to group vehicles.I've implemented this whole feature outside of the
DoCommand
system. I'm not sure if this is the best way, but since vehicle grouping is a purely UI feature (kind of a "view" over the real list of vehicles), it might be arguable that this resolution of contextual actions should also happen outside of the command system too. Also I'm not sure if there's a simpler way to implement this.How to test
Note: I'm not sure if checking that two vehicles have the same sequence of engines is the correct way to check if two vehicles are "equal" (for steps 4 and 5). Please correct me if it isn't!