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: Option to group vehicle list by shared orders #7028

Merged
merged 6 commits into from Dec 21, 2020

Conversation

btzy
Copy link
Contributor

@btzy btzy commented Jan 8, 2019

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:
image
image
image
image

Copy link
Member

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

src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/widgets/group_widget.h Outdated Show resolved Hide resolved
src/widgets/vehicle_widget.h Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
@btzy
Copy link
Contributor Author

btzy commented Jan 11, 2019

@LordAro Maybe I can split it into two commits: The first commit would split the original this->vehicles into this->vehicles and this->vehgroups. And the second commit would introduce GB_NONE and GB_SHARED_ORDERS. Would this be okay?

@btzy btzy force-pushed the station-vehicle-grouping branch 7 times, most recently from a31262a to 70ab77f Compare January 12, 2019 07:15
@LordAro LordAro requested a review from PeterN February 10, 2019 17:55
src/vehicle_gui.cpp Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Show resolved Hide resolved
@btzy btzy force-pushed the station-vehicle-grouping branch 3 times, most recently from 34cef8f to dbcac10 Compare February 21, 2019 07:00
@PeterN
Copy link
Member

PeterN commented Feb 28, 2019

Seems confusing to have both explicit groups and this. Not sure where they should fit together.

@PeterN
Copy link
Member

PeterN commented Feb 28, 2019

This doesn't compile currently:

src/vehicle_gui.cpp:204:73: error: initialized lambda captures are a C++14 extension [-Werror,-Wc++14-extensions] const Vehicle **end = std::find_if_not(begin, this->vehicles.End(), [first_shared = (*...

Plus it would be useful to rebase to master.

@btzy
Copy link
Contributor Author

btzy commented Feb 28, 2019

Seems confusing to have both explicit groups and this. Not sure where they should fit together.

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".)

@Eddi-z
Copy link
Contributor

Eddi-z commented Feb 28, 2019

I would maybe even go further and include other automatic group-like features like "all vehicles visiting <station>"

@btzy btzy force-pushed the station-vehicle-grouping branch 2 times, most recently from a578902 to d87bfd3 Compare February 28, 2019 13:59
@btzy
Copy link
Contributor Author

btzy commented Feb 28, 2019

I would maybe even go further and include other automatic group-like features like "all vehicles visiting "

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.

@btzy
Copy link
Contributor Author

btzy commented Feb 28, 2019

This doesn't compile currently:

src/vehicle_gui.cpp:204:73: error: initialized lambda captures are a C++14 extension [-Werror,-Wc++14-extensions] const Vehicle **end = std::find_if_not(begin, this->vehicles.End(), [first_shared = (*...

Plus it would be useful to rebase to master.

Sorry about that, now fixed and rebased to master.

@PeterN
Copy link
Member

PeterN commented Feb 28, 2019

Thanks, now able to test it :-)

PeterN
PeterN previously requested changes Mar 25, 2019
src/group_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui_base.h Outdated Show resolved Hide resolved
@PeterN
Copy link
Member

PeterN commented Mar 25, 2019

Could the group-by stuff be hidden/disabled when in the grouped-by vehicle list?

@btzy
Copy link
Contributor Author

btzy commented Mar 25, 2019

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?

@PeterN
Copy link
Member

PeterN commented Mar 25, 2019

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!

@btzy
Copy link
Contributor Author

btzy commented Mar 26, 2019

Here's how it looks when disabled:

image

@2TallTyler
Copy link
Member

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

@DorpsGek DorpsGek temporarily deployed to preview-pr-7028 December 17, 2020 08:12 Inactive
@btzy
Copy link
Contributor Author

btzy commented Dec 17, 2020

control-click-dragging a vehicle moves its entire shared order group

Wow, somehow I didn't notice that that worked!

@DorpsGek DorpsGek temporarily deployed to preview-pr-7028 December 17, 2020 12:30 Inactive
@btzy
Copy link
Contributor Author

btzy commented Dec 17, 2020

I've implemented the suggestion by @2TallTyler, it was a small change.

@TrueBrain
Copy link
Member

Indeed, in the station windows, the "Sort by" dropdown is above the "Group by" dropdown, but I didn't want to change the existing behaviour since that window is unrelated to this PR.

Oops, indeed, that Group by already exists. But it does leave us with an inconsistent UI (well .. more inconsistent then it was before your PR :P). I wonder if, for now, we should simply switch those two around to create a bit of consistency between the UIs .. that should be trivial to implement (I hope), and allows further work like the one you mention.

What are you thought on this? (feel free to disagree btw, I am just thinking out loud here).

@btzy
Copy link
Contributor Author

btzy commented Dec 19, 2020

@TrueBrain

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.

@TrueBrain
Copy link
Member

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!

@DorpsGek DorpsGek temporarily deployed to preview-pr-7028 December 19, 2020 17:22 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-7028 December 19, 2020 17:24 Inactive
@btzy
Copy link
Contributor Author

btzy commented Dec 19, 2020

@TrueBrain Done!

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.

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!

src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/group_gui.cpp Outdated Show resolved Hide resolved
This is in preparation for the new UI feature that allows grouping by shared orders.
@DorpsGek DorpsGek temporarily deployed to preview-pr-7028 December 20, 2020 04:05 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-7028 December 20, 2020 17:29 Inactive
@TrueBrain TrueBrain removed the work: minor details This Pull Request has some minor details left to do label Dec 21, 2020
@TrueBrain TrueBrain merged commit 981c540 into OpenTTD:master Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged preview This PR is receiving preview builds size: large This Pull Request is large in size; review will take a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants