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

Copy link
Contributor

@rubidium42 rubidium42 left a comment

Choose a reason for hiding this comment

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

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

src/widgets/network_widget.h Outdated Show resolved Hide resolved
src/network/network_gui.cpp Outdated Show resolved Hide resolved
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

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Contributor Author

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.

@telk5093

This comment has been minimized.

@TrueBrain TrueBrain merged commit f9b4a3a into OpenTTD:master Jun 28, 2021
@telk5093 telk5093 deleted the number_of_clients branch June 28, 2021 17:19
TrueBrain pushed a commit to TrueBrain/OpenTTD that referenced this pull request Jun 29, 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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants