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

A small change #633

Merged
merged 38 commits into from
Dec 19, 2023
Merged

A small change #633

merged 38 commits into from
Dec 19, 2023

Conversation

vishal332008
Copy link
Contributor

@vishal332008 vishal332008 commented Nov 13, 2023

  1. Fixed a screen message that no one will ever see.
  2. Displaying number of plugins added to the game.
  3. Displaying 'No Plugins Installed' if no plugins are there.
  4. Displaying the old messages as soon as you press 'Unmute Chat'.
  5. Added 'Add to Favorites' button in chat window for adding server to Favorites List.
  6. Displaying 'No Parties Added' if no parties are there in Favorites Tab.
  7. Showing the Characters of the specific names in Profiles Window.
  8. Added "Random Name" button (used replay icon) which gives random name to the player.
  9. Fixed a bug where no server is selected default in favourites tab.
  10. Fixed a bug where no replay is selected default in watch tab.
  11. Fixed a bug where no profile is selected default in profile tab.

A thing about point 2, I am displaying only the number and not text along with it because.. well because people have common sense. As soon as the player adds a plugin, the player will understand. So yeah.

And can you add seperate change logs for each point? If it is fine for you Eric.

Screenshot_20231122_111255
Screenshot_20231122_110737
Screenshot_20231126_134548
Screenshot_20231122_110631
Screenshot_20231123_180443

Copy link
Contributor

@EraOSBeta EraOSBeta left a comment

Choose a reason for hiding this comment

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

I think it's better to revert point 5 and make another PR for it after this one gets merged.

@vishal332008
Copy link
Contributor Author

vishal332008 commented Nov 18, 2023

It is okay to keep point 5 here itself. It is fine for me if Eric takes his time to pull this. And if Eric checks this PR, it will only take him like inside 1 hour to fix the problem of point 5. So I hope you can look past the problem @EraOSBeta . There are more good things than bad here.

@vishal332008 vishal332008 marked this pull request as draft November 19, 2023 12:21
@vishal332008 vishal332008 marked this pull request as ready for review November 19, 2023 12:21
Copy link
Contributor

@EraOSBeta EraOSBeta left a comment

Choose a reason for hiding this comment

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

Add to favorites button should be removed when we're the host (if this is possible right now).

Copy link
Contributor

@EraOSBeta EraOSBeta left a comment

Choose a reason for hiding this comment

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

The profile browser should load a fallback icon if a profile has an invalid character, it currently just throws an error and doesn't show any profiles if any of them have an invalid character.

@vishal332008
Copy link
Contributor Author

Add to favorites button should be removed when we're the host (if this is possible right now).

We didn't quite discuss the extent of add to favorites. I already said in the main comment (I edited it a few days before, so not your mistake if you didn't read it) that until Eric decides what he will add, it is waste of adding stuff which can be replaced. After Eric checks and says something about adding internal function or smth else, it's best to leave it like that.

The profile browser should load a fallback icon if a profile has an invalid character, it currently just throws an error and doesn't show any profiles if any of them have an invalid character.

Ok. Will add that.

@efroemling
Copy link
Owner

efroemling commented Nov 29, 2023

Some very nice additions; thanks!

Thoughts:

  • For point 5, I'd rather add a function to get the name/address of the current server. Could we take that one out for now and do it in a separate PR?
  • @EraOSBeta mentioned the profile-browser doesn't handle missing icons; could you fix that?
  • I think maybe just saying 'random' for the profile name button would work better; right now it looks a bit like it refreshes something.

Anyway, if you can make those few tweaks, I'd be happy to add whatever translation entries are needed and pull this in.

@vishal332008
Copy link
Contributor Author

Done

@EraOSBeta
Copy link
Contributor

EraOSBeta commented Dec 3, 2023

I think the title of this PR is no longer correctly representing this PR lol

@efroemling
Copy link
Owner

efroemling commented Dec 14, 2023

Ok I added bascenev1.get_connection_to_host_info_2() which will provide server name, address, and port (when connected). Let me know when you've got that wired up and I can pull this in 👍

@vishal332008
Copy link
Contributor Author

Can you release builds so I can actually test it?

@efroemling
Copy link
Owner

Ok new test builds are up.

@EraOSBeta
Copy link
Contributor

EraOSBeta commented Dec 15, 2023

Can you release builds so I can actually test it?

Note: from now on you can use CD for obtaining the newest test builds for all commits and PRs (excluding android builds)

@vishal332008
Copy link
Contributor Author

vishal332008 commented Dec 15, 2023

Ok. I finished the code of favourites button. Found a bug on the way. I fixed it in this PR's last commit. Now you can pull this one in so I can create new PR for favourites button.

@vishal332008
Copy link
Contributor Author

Or should I add the favourites button code in this PR itself?

@efroemling
Copy link
Owner

Or should I add the favourites button code in this PR itself?

Up to you. I'm ready to pull this PR in anytime so let me know if you'd prefer to add that here first or in a separate PR.

@vishal332008
Copy link
Contributor Author

I added it here itself. You can pull this PR now.

@efroemling efroemling merged commit fbee0d6 into efroemling:master Dec 19, 2023
15 checks passed
@efroemling
Copy link
Owner

Finally pulling this in. Thanks for all the improvements!
Do you by chance have a list of all the text resource strings I'll need to add?

@efroemling
Copy link
Owner

Actually I took a pass through and added the resource strings I saw. Just let me know if I missed anything.

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.

3 participants