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: Ctrl-Click on vehicle list in GB_SHARED_ORDERS opens order window #9325

Merged
merged 2 commits into from Sep 18, 2021

Conversation

btzy
Copy link
Contributor

@btzy btzy commented May 31, 2021

Motivation / Problem

To figure out which vehicle group we want (e.g. to clone) when looking at a vehicle list (e.g. at a station), we need to look at the order list. However, it takes 3 clicks (from "Bardingstone Heights - 48 trains" to "Train #83 (Orders)"), which is a bit unsatisfactory:

image

Description

Ctrl-Click on vehicle list window

This PR implements Ctrl-Click in the vehicle list window. Ctrl-Clicking on a vehicle in the vehicle list window (when "Group by" is set to "Shared orders") will open the order list of the first vehicle sharing those orders.

Issues with group GUI window

There is a slight change in behaviour for the group GUI window:

image

Ctrl-Clicking on a vehicle in the group window used to select the group in which the vehicle is in, but now it doesn't do that (when "Group by" is set to "Shared orders"). Well I think that was previously a bug - since vehicles from different groups could share orders, there may not be a unique group to select.

Issues with shared order list window

The shared orders window can't be grouped by shared orders, and it is pointless to do so anyway. But it still makes sense to want to open the order window to see the list of orders. Hence this PR adds a button at the top right corner to open the order list. The button is in a similar position to the the "Orders" button in the timetable window.

image

Unanswered questions

Should the order window have a title that feels more like the orders are shared, such as "Shared orders of Train #83 and 9 other vehicles" instead of "Train #83 (Orders)"? Since with shared orders, we no longer think of the order list as a property of a single vehicle, but instead logically each vehicle subscribes to some (shared) order list. Apart from the "Skip" button (and possibly the "Timetable" button which I'm not too familiar with), everything else pertains to things that are shared.

Note

With the addition of this PR, it is now possible to open the orders window without first opening the associated vehicle window. When closing a vehicle window, the associated orders window is automatically closed too, so it seems that previously such a situation could never happen. This PR doesn't seem to break anything though. But it does raise the question of whether we should head towards further decoupling between the order window and the vehicle window in the future, at least when the vehicle has shared orders.

In any case, this PR can be merged without answering the unanswered questions yet, so it isn't a blocking issue.

Limitations

Not that I know of.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@btzy btzy marked this pull request as draft May 31, 2021 08:12
@TrueBrain TrueBrain added the preview This PR is receiving preview builds label May 31, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-9325 May 31, 2021 08:16 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-9325 May 31, 2021 09:03 Inactive
@btzy btzy marked this pull request as ready for review May 31, 2021 09:10
@TrueBrain TrueBrain added this to the 12.0 milestone Sep 12, 2021
TrueBrain
TrueBrain previously approved these changes Sep 14, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Sorry it took a bit of time to get to this PR.

I have nothing against this PR, the motivation is reasonable, and it seems to help players, without hurting people that are used to their workflow. Basically, I do not see a reason why not.
I really do appreciate your lengthy explaining why you did what you did, that is very helpful. Tnx for that!

Should the order window have a title that feels more like the orders are shared, such as "Shared orders of Train #83 and 9 other vehicles" instead of "Train #83 (Orders)"? Since with shared orders, we no longer think of the order list as a property of a single vehicle, but instead logically each vehicle subscribes to some (shared) order list. Apart from the "Skip" button (and possibly the "Timetable" button which I'm not too familiar with), everything else pertains to things that are shared.

Yeah, this has been annoying me for a long time. I think #9551 is related. I do not think changing the caption now is a good idea, exactly for the reasons you mention (skip and timetable). But we really should address this whole Order window, as it is getting a bit out of hand :D But I will take my ideas to the discussion for that :)

src/vehicle_gui.cpp Outdated Show resolved Hide resolved
@btzy
Copy link
Contributor Author

btzy commented Sep 15, 2021

@TrueBrain Thanks for your review! I've rebased and fixed the nit. It does seem that rebasing dismisses your approving review...

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants