The International Simutrans Forum

 

Author Topic: [patch] access charge record and improvement of line management dialog  (Read 3590 times)

0 Members and 1 Guest are viewing this topic.

Offline Vladki

  • Devotee
  • *
  • Posts: 3328
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Hello. I'm looking at the new convoy management window. I like the new variants of convoy display, but I think the old way showing graphs and convoys together was more practical.

Offline Ranran

  • Devotee
  • *
  • Posts: 984
  • Languages: ja
As to the three sort buttons, may I suggest you make two of them as comboboxes, and the "Ascending/descending" button a checkbox instead?
What does the check box label look like? Does it mean "" (nothing)?

I think the old way showing graphs and convoys together was more practical.
But it is the same as the current standard. IMO, chart buttons occupy wasted space. And adding an access charge button made it more worse.
EDIT: Misunderstanding. I only saw prissi making such a suggestion, It didn't happen after all.

EDIT2:
And moved the broken livery scheme selector to the right. Because it is for convoys, not for stations.
« Last Edit: May 31, 2020, 02:39:13 AM by Ranran »

Offline Ranran

  • Devotee
  • *
  • Posts: 984
  • Languages: ja
I found a reproducible example of a crash, so I pull-requested a fix for it. Please check pull request #185.
I would appreciate it if you could confirm that it will not be reproduced on linux.

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Thank you: I have now integrated the fix. I should be grateful if Linux users could confirm whether this works.

Online Ves

  • Devotee
  • *
  • Posts: 1793
  • Languages: EN, SV, DK
Quote
What does the check box label look like? Does it mean "" (nothing)?
Just a checkbox with a small text:
[ x ] Reverse sort order.

Then the player only have to look if it is checked, instead of having to read the text in the button.

I like the new layout, I dont see a need to have the graphs visible as they were previously.



The only other thing I miss with the sorting options of the vehicles, is to display the value that they are sorting after. This is already true for Name, Next stop*, and Income, Loading level*, but is not true for: Max speed, Value, and Average age.

It would be nice if they would be displayed when the specific sort option is selected.

* These values are only indirectly displayed, or displayed when changing display mode. It would be nice if these values where displayed also in the "main" displaymode when the specific sort option is selected. I know there might be lenght issues, especially with the "next stop", but a solution to that could simply be to omit the "Destination:", saving alot of space.

Online prissi

  • Developer
  • Administrator
  • *
  • Posts: 9992
  • Languages: De,EN,JP
Just a word of caution. If the new GUI will be ported to extended, then all these alignments (and also taking care of overly large text) will be taken care by the GUI. I am not sure, if this is planned, but a lot of work would be wasted then.

Offline Freahk

  • Devotee
  • *
  • Posts: 1056
  • Languages: DE, EN
Imho, ascending/descending is one of very few places where the toggle button is still viable.
A small arrow icon toggle would be better, but we don't have such yet.

In any case as with most GUI decisions, personal preferences do often play huge role as there often is no clearly best solutions, it's rather a matter of what people are used to.

Offline Huitsi

  • *
  • Posts: 6
I'm afraid the crash is still happening.

Offline Ranran

  • Devotee
  • *
  • Posts: 984
  • Languages: ja
According to freddy's report, it crashes in a nightly, but it cannot be reproduced in a debug build. I didn't know the cause, so I removed the display of the tooltip that caused the previous build failure and crash. I would like to see if this causes a crash. Please check pull request # 186.

If you still get a crash, please report more details. Freddy said that selecting the second from the top of the wykstoke ferry company's line list from above would cause a crash, but that line didn't have a lot of convoy. Other reliable reproduction examples will be materials for narrowing down the conditions.

EDIT:
Note: Changes has not been incorporated yet.
« Last Edit: June 02, 2020, 09:04:21 AM by Ranran »

Offline freddyhayward

  • Devotee
  • *
  • Posts: 224
  • Languages: EN
If you still get a crash, please report more details. Freddy said that selecting the second from the top of the wykstoke ferry company's line list from above would cause a crash, but that line didn't have a lot of convoy. Other reliable reproduction examples will be materials for narrowing down the conditions.
That line did have 148 convoys when it caused the crash for me, which of course I could only find out using a debug build.

Offline Ranran

  • Devotee
  • *
  • Posts: 984
  • Languages: ja
Quote
Just a word of caution. If the new GUI will be ported to extended, then all these alignments (and also taking care of overly large text) will be taken care by the GUI. I am not sure, if this is planned, but a lot of work would be wasted then.
Thank you for your advice. It's not that I didn't consider it.
When a new function is added, it often involves changing the GUI. So I already see many small differences.
I found the standard depot filter patch useful, so I tried if I could apply it to extended. But it was difficult because the code was different than I imagined. The file structure is different. So I gave up it.
Another problem I have reported to standard is that we cannot type Japanese(double-byte characters) properly in extended (This happens under certain conditions). This was also the same.
This is a serious problem for Japanese player because all Japanese characters are double-byte characters.

Certainly, unnecessary work will be reduced if the GUI overhaul is already incorporated.
But I wonder if it is worth stopping because there is no guarantee that what I can do now will do in the future.
I don't know how many months or years later it will be. However I think the sooner it is, the better.

@James - Could you tell us about the progress and outlook for the incorporating from standard that phystam is promoting?

(Also, the smaller the difference from the standard, the more likely I can make a standard patch. Previously I tried compiling standard with MSVC but gave up. (´・ω・`))

The way I think about overhauling GUIs (if they differ a lot) is to incorporate the standard GUI as "empty". Next, make a frame. Then reposition the parts.





BTW, I made a sort button experimentally.

This is a simple one that uses existing functionality.
I was wondering if I could make the button turn light blue and white, but that wasn't possible with the existing features. I think we need to define a new button to do that. (´・ω・`)
The issue is that the gray button seems to be unavailable.
I'm wondering if it's worth doing because it's functional enough.
What do you guys think about this?


Quote
The only other thing I miss with the sorting options of the vehicles, is to display the value that they are sorting after. This is already true for Name, Next stop*, and Income, Loading level*, but is not true for: Max speed, Value, and Average age.

It would be nice if they would be displayed when the specific sort option is selected.
I'm thinking of using the third line of empty.


Quote
I know there might be lenght issues, especially with the "next stop", but a solution to that could simply be to omit the "Destination:", saving alot of space.
Yes, that's exactly why I didn't show it on the empty third line.
We must specify the number of characters to cut off the display, not the width. That was troublesome. (´・ω・`)

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Thank you for that: I have now incorporated the changes; I should be grateful if anyone could test with to-morrow's nightly build whether this fixes the crash.

As to Phystam's work in incorporating changes from Standard, I am afraid that I know nothing more than is on the thread in the public forum regarding this; I had asked whether the work was compatible with the private car route finding code some months ago, and have not had an answer to this question and there appears to have been no progress since.

On a different topic, I do like the up and down buttons instead of the large button with the words "ascending" and "descending": this is clearer and easier, I think. I note that this is not in the latest commit to your branch, but perhaps it was not intended to be yet?

In terms of Japanese characters, I was not aware of this until now. Is this not an issue in Standard? If this has been resolved in Standard, it might be worth finding the specific resolution for this and backporting this as a higher priority than the general work, since it is a problem if Japanese speakers cannot type characters in Japanese properly.

Online Ves

  • Devotee
  • *
  • Posts: 1793
  • Languages: EN, SV, DK
I like alot the combobox sort option, I wondered if it would also not be usefull to have a combobox ínstead of the right hand button as well (the one labeled cl_btn_general)?
Also interresting with the up and down arrows, had not thought of that idea!

Quote
I'm thinking of using the third line of empty.
Do you mean that  you consider doing it on the third line, or that you want it to remain empty?  :P

Quote
Yes, that's exactly why I didn't show it on the empty third line.
We must specify the number of characters to cut off the display, not the width. That was troublesome. (´・ω・`)
Have a look in the convoy info window for the line entry. I made my own character limiting display there, asuming that every character was five pixels wide, and then I added the rest of the characters needed (arrows, reverse symbol etc). I have since learned that there might be better options than to just assume a character to be 5 pixels, but perhaps that approach can be used?

Offline Ranran

  • Devotee
  • *
  • Posts: 984
  • Languages: ja
Quote
On a different topic, I do like the up and down buttons instead of the large button with the words "ascending" and "descending": this is clearer and easier, I think. I note that this is not in the latest commit to your branch, but perhaps it was not intended to be yet?
Yes, because I wanted to ask first if there was a problem with this style.
I have now pushed it to the same branch so you can check it working.

If there is no problem with this style, other dialogs can be replaced as well.


Quote
Is this not an issue in Standard? If this has been resolved in Standard, it might be worth finding the specific resolution for this and backporting this as a higher priority than the general work, since it is a problem if Japanese speakers cannot type characters in Japanese properly.
Yes, it was a standard issue solved in June 2018. The thread is here. It will be a long way incorporating from the standard in order. A particular problem is when entering double-byte characters in markers.


Do you mean that  you consider doing it on the third line, or that you want it to remain empty?
Meaning I'm considering doing it on the third line :-[

Quote
I wondered if it would also not be usefull to have a combobox ínstead of the right hand button as well (the one labeled cl_btn_general)?
I'm still thinking about what to do with this button, including tabbing, so I haven't started yet.


Quote
Have a look in the convoy info window for the line entry. I made my own character limiting display there, asuming that every character was five pixels wide, and then I added the rest of the characters needed (arrows, reverse symbol etc). I have since learned that there might be better options than to just assume a character to be 5 pixels, but perhaps that approach can be used?
At least one Japanese/Chinese/Korean is more than 5px wide. Also, font sizes will be variable with incorporating from standard.[

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Ranran - are you able to locate the individual commit on the SVN's Github mirror where the Japanese character fix occurs? I might be able to port this manually if this is not too heavily connected to code that has diverged fundamentally between Extended and Standard.

As to the dialogue improvement, this is much clearer and easier to use and I will incorporate this: thank you. One suggestion, however, is to add a tooltip text for the buttons to show "ascending" and "descending"; you can even re-use the text from the buttons so that no new translations should be necessary.

Offline Ranran

  • Devotee
  • *
  • Posts: 984
  • Languages: ja
Ranran - are you able to locate the individual commit on the SVN's Github mirror where the Japanese character fix occurs? I might be able to port this manually if this is not too heavily connected to code that has diverged fundamentally between Extended and Standard.
Here it is.
https://github.com/aburch/simutrans/commit/b4849504fb4f7f123f674a377e6f0128e8c58056
Since it was two years ago, my memory may have been incorrect. Is it easy to incorporate this in?


One suggestion, however, is to add a tooltip text for the buttons to show "ascending" and "descending"; you can even re-use the text from the buttons so that no new translations should be necessary.
This was simply my miss. In addition to that fix, I made a pull request for the other ascend/descend buttons of other conboboxed list dialogs. Please check pull request #187.

Offline Vladki

  • Devotee
  • *
  • Posts: 3328
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
The line info  windows shows Revenue: number, but in fact it is Profit:   Compare with graphs.
Revenue cannot be negative....

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Here it is.
https://github.com/aburch/simutrans/commit/b4849504fb4f7f123f674a377e6f0128e8c58056
Since it was two years ago, my memory may have been incorrect. Is it easy to incorporate this in?

Thank you very much for finding this. Fortunately, this was easy to incorporate and I have now incorporated this. I should be grateful if you could re-test.

Offline Huitsi

  • *
  • Posts: 6
It seems the crash has not gotten fixed yet. The two lines that cause the crash are "Wykstoke Ferry" and "Anningdale - Pyewater trunk ferry".

Offline freddyhayward

  • Devotee
  • *
  • Posts: 224
  • Languages: EN
I've made a pull request that fixes a crash when clicking the livery selector (applicable to all combo-boxes) with no available liveries. I'm also looking at other oddities related to the livery selector.

Offline freddyhayward

  • Devotee
  • *
  • Posts: 224
  • Languages: EN
It seems the crash has not gotten fixed yet. The two lines that cause the crash are "Wykstoke Ferry" and "Anningdale - Pyewater trunk ferry".
Same with me. Ranran - the tooltips can't be the cause of this, so it would be good to have them back.

Offline Ranran

  • Devotee
  • *
  • Posts: 984
  • Languages: ja
It seems the crash has not gotten fixed yet. The two lines that cause the crash are "Wykstoke Ferry" and "Anningdale - Pyewater trunk ferry".
https://drive.google.com/file/d/13Iv6WdUnx6k0GErx1icZUXXdr8SND6tn/view?usp=sharing
https://drive.google.com/file/d/1r6zLgcEzjB-RflfJUf6xAUanjBmHShoK/view?usp=sharing
The reason seems to be that there are many convoys. Can these saves open the line management dialog?

Offline Huitsi

  • *
  • Posts: 6
Both crash when trying to look at the (1) line.

Offline Ranran

  • Devotee
  • *
  • Posts: 984
  • Languages: ja
Thank you for confirmation.
Can you open the Vehicle list? They use a common view, and the Vehicle list shows more.

Offline Huitsi

  • *
  • Posts: 6
Yes, the vehicle list opens and works without crashing.

Offline freddyhayward

  • Devotee
  • *
  • Posts: 224
  • Languages: EN
I've submitted a pull request that fixes the crash, at least for linux. There might be something I was missing though, Ranran - for some reason you made 'char tmp_buf[5]' instead of directly formatting the label (which is what my pull request does). What was the purpose of this?

EDIT: the problem was that a char[5] is only enough to store 4 characters plus the end character. 2 characters were used for '(' and ')', so only 2 extra digits could be stored properly. It seems that linux is much more strict with memory, probably for the best, and did not allow this.

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Thank you - incorporated.

Offline Ranran

  • Devotee
  • *
  • Posts: 984
  • Languages: ja
Thank you for finding it.
Quote
Ranran - for some reason you made 'char tmp_buf[5]' instead of directly formatting the label (which is what my pull request does). What was the purpose of this?
This was to avoid breaking the position where player click the tab and react if you do not fix the width of the character. Therefore, I tried to secure a 3-digit display so that the character length would not change.
For example, when the number of convoys changes from 1 digit to 2 digits and the character length changes, the tab position changes, but the place where the click responds does not change.
Therefore, clicking the tab sometimes did not seem to work.

Offline freddyhayward

  • Devotee
  • *
  • Posts: 224
  • Languages: EN
Thank you for finding it.This was to avoid breaking the position where player click the tab and react if you do not fix the width of the character. Therefore, I tried to secure a 3-digit display so that the character length would not change.
For example, when the number of convoys changes from 1 digit to 2 digits and the character length changes, the tab position changes, but the place where the click responds does not change.
Therefore, clicking the tab sometimes did not seem to work.
I see, I just tested switching from a 1000-convoy line to a 1-convoy line and this indeed happens. I tried reverting to before my patch and changing tmp_buf[5] to tmp_buf[8] and %5s to %7s. The problem still occurs because the characters are not fixed width, so the spaces are much narrower than the digits. Would it be too much to re-compute the width of the tab every time the tab name changes?

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
We should not really be using limitations on memory storage to impose a restriction on the number of characters that can be displayed, as this is unstable: instead, an algorithm to truncate a string if necessary should be deployed.

Offline Freahk

  • Devotee
  • *
  • Posts: 1056
  • Languages: DE, EN
On top of that, abusing C-style char arrays, instead of std::string is highly deprecated in C++ because of such issues.

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
On top of that, abusing C-style char arrays, instead of std::string is highly deprecated in C++ because of such issues.

Although much of the Simutrans codebase was written before this was widely understood.

Offline Freahk

  • Devotee
  • *
  • Posts: 1056
  • Languages: DE, EN
Yes, I'm not blaming anyone for such code to exist. It was simply usual to do it that way in old C times, and there still are some very rare cases where this might be preferable.

I just wanted to point out that there are very good reasons not to use char arrays in most scenarios, so new code should not stick to these old practices and in case of bugs cause by such, the bugfix should attemt to circumvent the use of very dangerous char arrays either. For sure, not at all costs.

Online prissi

  • Developer
  • Administrator
  • *
  • Posts: 9992
  • Languages: De,EN,JP
You are fighting a lot of bugs that Dwachs new autoscaling GUI code solved, by the way. Liek the combobox issue or truncating strings of there is not enough space and much more. Just an observation. Enabling the new GUI code does not require to change any old dialoge. But it offers the chance to make any new dialoge using the new scalable GUI and thus avoids double work.

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
You are fighting a lot of bugs that Dwachs new autoscaling GUI code solved, by the way. Liek the combobox issue or truncating strings of there is not enough space and much more. Just an observation. Enabling the new GUI code does not require to change any old dialoge. But it offers the chance to make any new dialoge using the new scalable GUI and thus avoids double work.

If anyone would like to work on integrating this, it would be most welcome, although I suspect that it might be a lot of work.