-
-
Notifications
You must be signed in to change notification settings - Fork 947
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: Option to group vehicle list by shared orders #7028
Conversation
a02f906
to
7d4722c
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.
General idea is great, thanks for contributing! It's quite a large diff with lots of changes all together, do you think it would be feasible to split it into multiple logical commits?
I wonder about the fairly significant use of C++11 stuff as well. Technically we should be all fine for it, but...
@LordAro Maybe I can split it into two commits: The first commit would split the original |
a31262a
to
70ab77f
Compare
34cef8f
to
dbcac10
Compare
Seems confusing to have both explicit groups and this. Not sure where they should fit together. |
This doesn't compile currently:
Plus it would be useful to rebase to master. |
My original motivation was that it was difficult to find a correct vehicle (for cloning / Ctrl-clicking) in a long list. I tend to play with lots of vehicles, and without this patch I would have to click vehicles at random and open their order lists to see if I've gotten the correct one. Grouping by shared orders also allows certain contextual actions (on the vehicle list) that explicit groups can't have, such as opening the "Shared orders of X vehicles" window, opening the order list, or Ctrl-clicking for cloning. Since both explicit groups and shared orders already exist in OpenTTD and solve orthogonal concerns (explicit groups being arbitrarily user-defined and shared orders being defined by having the same route), maybe it would reduce confusion to have a third option in the "group by" dropdown that groups by the explicit (user-defined) group? This would probably emphasise the distinction between explicit groups and shared orders. (If this is done, maybe "user-defined group" is a better term than "vehicle group".) |
I would maybe even go further and include other automatic group-like features like "all vehicles visiting <station>" |
a578902
to
d87bfd3
Compare
I'm not sure how this could be a group-like feature. Vehicles can visit multiple stations, so this kind of grouping doesn't seem to be mutually exclusive? Looks more like a filter-like feature to me. |
Sorry about that, now fixed and rebased to master. |
Thanks, now able to test it :-) |
Could the group-by stuff be hidden/disabled when in the grouped-by vehicle list? |
I'm not sure which list you are referring to... do you mean the "Shared orders of X vehicles" list? |
Yes, that particular one. I'm not sure if it makes sense or not to be have to have it grouped by shared orders, as it already is shared orders. On the other hand, if other group-by schemes are added it would again make sense. Hmm! |
d87bfd3
to
aed7b51
Compare
I don't quite understand that code, but in current trunk, control-click-dragging a vehicle moves its entire shared order group. Could you use that behavior? (perhaps selecting the first vehicle in the group, if you can't directly select all of them). |
1a9e596
to
30f90e9
Compare
Wow, somehow I didn't notice that that worked! |
I've implemented the suggestion by @2TallTyler, it was a small change. |
Oops, indeed, that What are you thought on this? (feel free to disagree btw, I am just thinking out loud here). |
I agree that switching the "Group by" with "Sort by" dropdowns in the station window is strictly better than the current version, since the "correct" use of "Sort by" (i.e. by station name or by amount) should logically come after the grouping option. Should I implement this in the same PR? I'm still not fully convinced that my three-level menu suggestion is ideal though. While being the most hierarchically accurate (in my opinion), it takes up another row of valuable screen space for something that most users will never use, or only use once... I, myself, when playing this game, have never changed the sorting option from "Station: Waiting", and have set the grouping option to "Via-Destination-Source" when cargodist was added and left it there ever since. |
If you wouldn't mind adding the switching of the two buttons in this PR (as a separate commit) that would be lovely; avoids us forgetting about it or creating a new PR for only just that :) We can debate in a later PR or Discussion what we should do with those dropdowns, as I agree that adding yet-another-button is not going to help out the UI :D Tnx! |
546114a
to
be70953
Compare
be70953
to
b12e599
Compare
@TrueBrain Done! |
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.
Some minor things, nothing fancy. Almost there, I would say :)
Mostly, some comments confused me more than they helped me understand the code. Maybe you can look at those and see if you can add some letters (read: words) to make it more obvious what the intention was :)
Tnx!
This is in preparation for the new UI feature that allows grouping by shared orders.
b12e599
to
a21399c
Compare
This applies to all kinds of vehicle lists, as well as the "vehicle groups" window.
a21399c
to
7feb238
Compare
I added a "group by" dropdown to all vehicle lists to display one row per shared order, instead of one row per vehicle. I've tested it with all vehicle types, as well as with the "vehicle groups" window. When grouped by shared orders, clicking on a row will open the "shared orders of X vehicles" window.
Rationale: Quite often, we are concerned only with "routes" instead of individual vehicles. This feature highlights the important information (i.e. the routes) while reducing the importance of non-important information (i.e. the individual vehicles).
Extension: It is possible to have a third option - to group by vehicle group. As vehicle groups are mutually exclusive, it should be relatively simple to implement this extension. However, I'm not sure of its usefulness and I didn't want to make this PR too big.
Note: The new dropdown is available in the "shared orders of X vehicles" window too. Though it is of not much use, I left it in for consistency. The dropdown will be useful for the extension described above.
This is as per my post on TT Forums: https://www.tt-forums.net/viewtopic.php?f=32&t=84587&sid=fa68e80118896fca8f275a70d5cf6045
Some screenshots:



