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: Show the number of clients and companies in the online players window #9376

Merged
merged 4 commits into from Jun 28, 2021

Conversation

@telk5093
Copy link
Contributor

@telk5093 telk5093 commented Jun 16, 2021

Motivation / Problem

Thanks to @TrueBrain, we have a new Online players window by 5266359
It is nice, but hard to know how many clients and companies at once during playing a multiplayer game.
So I added an additional line to display the total number of clients and companies.

Description

This feature adds the number of clients and companies at the bottom of the online players window like screenshot below:
image

Limitations

  • There is a problem with displaying number of clients/companies in RTL languages, such as Arabic.
    I have no idea whether my code is wrong, or it will be fixed when Arabic and RTL language translators make a translation.
    image

  • I'm not good at C++ and OpenTTD's code so please let me know if any problems or suggestions. Thanks beforehand!

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')
@telk5093 telk5093 force-pushed the number_of_clients branch from 8803509 to fe5b8ae Jun 16, 2021
Copy link
Contributor

@rubidium42 rubidium42 left a comment

  • There is a problem with displaying number of clients/companies in RTL languages, such as Arabic.
    I have no idea whether my code is wrong, or it will be fixed when Arabic and RTL language translators make a translation.

It's expected that it looks weird due to all kinds of different rules for ordering different things. There's nothing you can do about it, however the translators can inject special characters to ensure the string is shown correctly when translated.

It's a nice first attempt, though there are some things that could be improved to make it clearer and simpler overall.

Loading

src/widgets/network_widget.h Outdated Show resolved Hide resolved
Loading
src/network/network_gui.cpp Outdated Show resolved Hide resolved
Loading
NWidget(NWID_VSCROLLBAR, COLOUR_GREY, WID_CL_SCROLLBAR),
EndContainer(),
NWidget(NWID_HORIZONTAL),
NWidget(WWT_TEXTBTN, COLOUR_GREY, WID_CL_CLIENT_NUMBER), SetFill(1, 0), SetMinimalTextLines(1, 0), SetResize(1, 0), SetAlignment(SA_RIGHT),
Copy link
Contributor

@rubidium42 rubidium42 Jun 17, 2021

Choose a reason for hiding this comment

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

Shouldn't this be a WWT_LABEL or maybe WWT_TEXT? It's not a button you are supposed to click on, right?

Loading

Copy link
Contributor Author

@telk5093 telk5093 Jun 17, 2021

Choose a reason for hiding this comment

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

It was my intend to draw the container of the count of clients/companies as a box shape.
Anyway I changed WWT_TEXTBTN into WWT_TEXT now, though it looks weird for me.
Please let me know if there is a better idea.

Loading

Copy link
Contributor

@rubidium42 rubidium42 Jun 19, 2021

Choose a reason for hiding this comment

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

It definitely looks different, though I think it looking different is not a bad thing as it makes it more clear it is not one of the lines in the table, but rather a footer. With the text button it looked like it was part of the table.
Having said that, I think it looks better when the string is aligned in the center (SA_CENTER), as the right alignment looks weird to me. Though, that can also be my bad taste. Maybe some others have an opinion they want to share about the looks of the button and alignment of the text.

Loading

Copy link
Contributor

@embeddedt embeddedt Jun 20, 2021

Choose a reason for hiding this comment

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

I agree that right-alignment looks strange here. It should either be centered or left-aligned, IMO. Left alignment probably makes more sense, since this is not a button.

Loading

Copy link
Contributor Author

@telk5093 telk5093 Jun 20, 2021

Choose a reason for hiding this comment

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

Okay, let's do them.

Left:
image

Centre:
image

I like whatever and so I can't decide it.

Loading

Copy link
Contributor

@glx22 glx22 Jun 20, 2021

Choose a reason for hiding this comment

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

Center is not bad, left is OK too, both are better than right. If I have to decide I'd say center.
BTW it seems the matrix is longer than the scroll bar.

Loading

Copy link
Member

@TrueBrain TrueBrain Jun 27, 2021

Choose a reason for hiding this comment

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

Center looks really nice in my book :D

Loading

Copy link
Contributor Author

@telk5093 telk5093 Jun 28, 2021

Choose a reason for hiding this comment

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

OK, now make it centered.

Loading

src/network/network_gui.cpp Show resolved Hide resolved
Loading
@telk5093

This comment has been hidden.

@TrueBrain TrueBrain merged commit f9b4a3a into OpenTTD:master Jun 28, 2021
15 checks passed
Loading
@telk5093 telk5093 deleted the number_of_clients branch Jun 28, 2021
TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Jun 29, 2021
Taschi120 added a commit to Taschi120/OpenTTD that referenced this issue Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants