The International Simutrans Forum

Simutrans Extended => Patches/pull requests for consideration => Simutrans-Extended development => Incorporated patches/merged pull requests => Topic started by: Ranran(retired) on May 09, 2019, 02:28:31 PM

Title: [patch] Correct cargo list redundancy
Post by: Ranran(retired) on May 09, 2019, 02:28:31 PM
Hi. Ranran made a small patch. (´・ω・`)

The cargo list is currently cluttered by unnecessarily repeating the same word.
This patch reduces duplicate text, reduces the amount of text, and makes the list slightly sexy slimmer.



Examples:
The left side is after the change and the right side is before the change.

For example, everyone knows that everything grouped by passengers is passengers.
(https://i.imgur.com/d4tNbfr.png)

(https://i.imgur.com/Bi6a1MI.png)


However, in some cases, it can subdivide passengineers further in Extended. In that case, it displays like this.
(https://i.imgur.com/PW4yAcM.png)

(https://i.imgur.com/cBmWre9.png)



The freight goods display is as it is.


The repository for this patch is here.
https://github.com/Ranran-the-JuicyPork/simutrans-extended/tree/slim-the-cargo-list


What do you guys think about this? (´・ω・`)
Title: Re: [patch] Correct cargo list redundancy
Post by: Matthew on May 09, 2019, 10:22:04 PM
This looks like two really good ideas!  :D

Firstly, saving 'screen estate' (space on the screen) by not using the word "passengers" unnecessarily.

Secondly, making it possible for players to distinguish Commuters from Visitors.

IMHO it would definitely be an improvement.
Title: Re: [patch] Correct cargo list redundancy
Post by: wlindley on May 10, 2019, 05:15:04 PM
Excellent!  While you are looking at those bits of code, I wonder whether you would agree that sorting by "Via (amount)" which currently gives results like:
30 via A
25 to B
20 to C
15 via B
5 via C

would be more useful if it told us how many passengers wanted to travel either to or via, like this:
40 via B (25 to, 15 via)
30 via A
25 via C (20 to, 5 via)

especially as that is far more useful in determining where to add service.  Listing the "to" and "via" separately seems quite confusing, don't you think?
I'm not sure whether that would be simple or complex to change.
Title: Re: [patch] Correct cargo list redundancy
Post by: Ves on May 10, 2019, 11:35:41 PM
Thank you, that looks really good!  :D
Title: Re: [patch] Correct cargo list redundancy
Post by: ACarlotti on May 11, 2019, 02:39:21 AM
One specific comment: In lines 577-584, you should replace the current construction (if (a&&!b) {} else if (a&&b) {} else {}) with nested ifs (i.e. if (a) { if (b) {} else {}} else {}).

Another issue I've noticed is that the code ignores that some languages have different pluralisation schemes. However, I suspect this is probably a widespread existing problem, and therefore beyond the scope of these changes.

Quote from: wlindley on May 10, 2019, 05:15:04 PMListing the "to" and "via" separately seems quite confusing, don't you think?
I'm not sure whether that would be simple or complex to change.

I think it would be relatively complicated to combine them while still showing the separate numbers, but relatively simple to combine them with just a single total.
Title: Re: [patch] Correct cargo list redundancy
Post by: jamespetts on May 13, 2019, 11:17:08 PM
Thank you for this - this is definitely an improvement.

One thing that I do notice when testing, however, is that, in "wealth (detail)" and "destination (detail)" modes, when passengers/mail/goods are going to a destination that is not in a town, an empty pair of brackets are displayed where the town name would usually go. Are you able to recode this so that the brackets show only when the town is non-null?

Thank you again.
Title: Re: [patch] Correct cargo list redundancy
Post by: Ranran(retired) on May 14, 2019, 12:57:29 PM
Thank you for your feedbacks.  :)

Quote from: ACarlotti on May 11, 2019, 02:39:21 AMOne specific comment: In lines 577-584, you should replace the current construction (if (a&&!b) {} else if (a&&b) {} else {}) with nested ifs (i.e. if (a) { if (b) {} else {}} else {}).
Quote from: jamespetts on May 13, 2019, 11:17:08 PMOne thing that I do notice when testing, however, is that, in "wealth (detail)" and "destination (detail)" modes, when passengers/mail/goods are going to a destination that is not in a town, an empty pair of brackets are displayed where the town name would usually go.
Thank you for pointing it out. I think they were the same issue. I think now I fixed it.


Quote from: wlindley on May 10, 2019, 05:15:04 PMI wonder whether you would agree that sorting by "Via (amount)" which currently gives results like:

30 via A
25 to B
20 to C
15 via B
5 via C

would be more useful if it told us how many passengers wanted to travel either to or via, like this:

40 via B (25 to, 15 via)
30 via A
25 via C (20 to, 5 via)

especially as that is far more useful in determining where to add service.  Listing the "to" and "via" separately seems quite confusing, don't you think?
I'm not sure whether that would be simple or complex to change.
Quote from: ACarlotti on May 11, 2019, 02:39:21 AMI think it would be relatively complicated to combine them while still showing the separate numbers, but relatively simple to combine them with just a single total.
I agree with wlindley's idea. :award: I have integrated these based on the second proposal from ACarlotti.
Please check the following comparison picture.

(https://i.imgur.com/bACbePY.png)

It is very useful to know where convoy passengers get off and where passengers waiting at the station are getting off.
IMO, I don't think it's necessary to divide this into via and to in this case. If player want to distinguish between "via" and "to", via (detail) will let you know, so I don't think there is any problem with integrating them in via (amount).
Title: Re: [patch] Correct cargo list redundancy
Post by: jamespetts on May 16, 2019, 10:24:11 PM
Thank you very much for this: this is good progress.

Two remaining things: first of all, in "Wealth (via"), we still have occasions when we get "> via", which is redundant. Relatedly, all the instances of "via" have been replaced with ">"; might it not be clearer to have "via" instead of ">" in the "via" modes? Then, we have ">" for "to", "<" for "from" and "via".

Thank you again for your work on this.
Title: Re: [patch] Correct cargo list redundancy
Post by: Ranran(retired) on May 17, 2019, 04:18:28 PM
I made some corrections. Is this adjustment in line with your requirements?

EDIT:
Currently, the wealth(via) is sorted by the name of the stop, but I thought it would be more convenient to order by the amount like this.
(https://i.imgur.com/jTBBSwY.png)

This change can be found in the branch below.
https://github.com/Ranran-the-JuicyPork/simutrans-extended/tree/slim-the-cargo-list2
Title: Re: [patch] Correct cargo list redundancy
Post by: jamespetts on May 17, 2019, 10:35:25 PM
Thank you very much for this. I do prefer the amount sorting for wealth (via), so I have included this on the cargo-list-improvements branch.

However, I am noticing some problems with via (amount): this seems to be showing the destination rather than the next transfer in at least some cases. I wonder whether you might look into this?

Thank you again for your work on this.
Title: Re: [patch] Correct cargo list redundancy
Post by: Ranran(retired) on May 18, 2019, 03:57:26 AM
Quote from: jamespetts on May 17, 2019, 10:35:25 PMI wonder whether you might look into this?
Thank you for pointing it out. I think I've fixed it now.
Title: Re: [patch] Correct cargo list redundancy
Post by: jamespetts on May 18, 2019, 09:25:01 AM
Excellent, thank you very much: I have now confirmed that this fix works.

This has now been incorporated into the master branch, so will be available from the next nightly build. Thank you very much for your work on this: it is much appreciated.