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

Feature: Contextual actions for vehicles grouped by shared orders #8425

Merged
merged 1 commit into from Nov 26, 2022

Conversation

btzy
Copy link
Contributor

@btzy btzy commented Dec 24, 2020

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

  1. Load a game, open the vehicle list of some station with many routes passing through it, and set the vehicle list to group by shared orders.
  2. Create a new vehicle, open the order list, click "Go to", then click any row in the vehicle list from step 1. Observe that the order list is copied to the new vehicle.
  3. Same as step 2, but hold down CTRL when clicking the row in the vehicle list. Observe that the new vehicle is now sharing orders.
  4. Create a depot, click "Clone Vehicle", then click any row in the vehicle list from step 1. If all vehicles in the vehicle list have the same sequence of engines, then the vehicle will be cloned. Otherwise an error will be displayed.
  5. Same as step 4, but hold down CTRL when clicking the row in the vehicle list. You should observe the same thing as step 4, but the new vehicle is now sharing orders.

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!

@2TallTyler 2TallTyler added the work: needs rebase This Pull Request needs a rebase label Oct 19, 2022
@2TallTyler 2TallTyler added the preview This PR is receiving preview builds label Nov 7, 2022
@DorpsGek DorpsGek temporarily deployed to preview-pr-8425 November 7, 2022 21:06 Inactive
@2TallTyler
Copy link
Member

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. 😄

@btzy
Copy link
Contributor Author

btzy commented Nov 8, 2022

Oh wow a two-year response time ;)

I'll try to rebase it later tonight.

@2TallTyler
Copy link
Member

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.

@DorpsGek DorpsGek temporarily deployed to preview-pr-8425 November 8, 2022 17:22 Inactive
@btzy
Copy link
Contributor Author

btzy commented Nov 8, 2022

@2TallTyler Done rebasing.

@DorpsGek DorpsGek temporarily deployed to preview-pr-8425 November 8, 2022 18:17 Inactive
@2TallTyler
Copy link
Member

There were some conflicts and the CI checks didn't run so I rebased. I'll try to review tomorrow.

@DorpsGek DorpsGek temporarily deployed to preview-pr-8425 November 9, 2022 16:32 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8425 November 9, 2022 16:48 Inactive
@btzy
Copy link
Contributor Author

btzy commented Nov 9, 2022

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.

src/depot_gui.cpp Show resolved Hide resolved
src/depot_gui.cpp Outdated Show resolved Hide resolved
src/depot_gui.cpp Outdated Show resolved Hide resolved
src/depot_gui.cpp Show resolved Hide resolved
src/order_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Show resolved Hide resolved
src/vehicle_gui.cpp Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/window_gui.h Outdated Show resolved Hide resolved
src/window_gui.h Show resolved Hide resolved
@2TallTyler 2TallTyler added size: small This Pull Request is small, and should be relative easy to process and removed work: needs rebase This Pull Request needs a rebase labels Nov 9, 2022
@DorpsGek DorpsGek temporarily deployed to preview-pr-8425 November 10, 2022 15:32 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8425 November 10, 2022 15:41 Inactive
@btzy
Copy link
Contributor Author

btzy commented Nov 10, 2022

Thanks for the review!

@DorpsGek DorpsGek temporarily deployed to preview-pr-8425 November 10, 2022 15:49 Inactive
@btzy btzy requested a review from 2TallTyler November 12, 2022 05:30
@DorpsGek DorpsGek temporarily deployed to preview-pr-8425 November 12, 2022 05:31 Inactive
Copy link
Member

@2TallTyler 2TallTyler left a 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.

src/depot_gui.cpp Show resolved Hide resolved
src/window_gui.h Show resolved Hide resolved
@michicc michicc merged commit 8a78fa7 into OpenTTD:master Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants