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

Add visited servers section to serverlist #15660

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

siliconsniffer
Copy link
Contributor

@siliconsniffer siliconsniffer commented Jan 8, 2025

Add compact, short information about your PR for easier understanding:

This PR aims to add a new section into the server list to display the last visited servers in chronological order from newest to oldest descending.

Goal of the PR

The goal is to stop the automatic addition of servers into the favorites section when joining them.

How does the PR work?

When joining a non-favorite server it will be added to the first/top entry of the "last visited" section.
Additionally the section is limited to only five entries, if there are five entries and there is a new one the last will be removed and the new one will be displayed at the top of the list.
The behavior of "doing nothing" (not adding the server to favs) when double clicking is kept - it won't be added to last visited.

Does it resolve any reported issue?

#4028 (maybe more?)
#15545 second and third point, first point could be solved here too?

Does this relate to a goal in the roadmap?

2.3 hopefully

If not a bug fix, why is this PR needed? What usecases does it solve?

It solves the annoyance of having every server one joins in the favorites.
A nice side effect is that the selection is kept when searching - a user has to specifically pick a server from the results to select it.

To do

This PR is a Work in Progress.
I don't mind suggestions for how to fix the sorting/make them sorted chronically instead of being sorted by their rank - my attempt doesn't seem to work out as I expected. I'd say its ready for review when that works.

  • ignore server ranking, sort servers chronologically
  • discuss icon?

How to test

Try to join a few servers, observe how the "Last Visited" section fills up.
Notice how it will remove the oldest entry if there are already five and a new one is added.
Double click on a server to connect to see that it doesn't get added to the list.
See that favorite servers won't get added to the list.

Preview

image

@sfan5 sfan5 added Feature ✨ PRs that add or enhance a feature @ Mainmenu labels Jan 8, 2025
@LoneWolfHT
Copy link
Contributor

LoneWolfHT commented Jan 8, 2025

Idea: Add a checkbox at the top of the serverlist for controlling the display of the 'Last Visited' (maybe better named 'Recently Visited' or 'Visited Previously'?) section. Moving all the 'Last Visited' server entries to the 'Public Servers' section when disabled.
The goal would be to allow people to view the normal serverlist rankings without the servers they've visited before being left out

An alternative to the checkbox would maybe be having the user click/doubleclick the 'Last Visited' header to toggle it, turning the text grey when disabled or something

@siliconsniffer siliconsniffer marked this pull request as ready for review January 9, 2025 08:44
@sfan5 sfan5 added the Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand label Jan 9, 2025
@cx384
Copy link
Contributor

cx384 commented Jan 9, 2025

Some other games I know use tabs for the server list, to select between e.g. Public, Lan, Favorites, History
I think this is a better solution than putting them in a single scrollable box.
So I suggest to not only make it a binary checkbox, and instead use e.g. a dropdown or something more fancy tab like or with multiple buttons to select between, Public Servers, Favorites, Last Visited.
(Only one active at the same time.)

This would also be more future proof if we get #10208 and #14246 someday.

@siliconsniffer
Copy link
Contributor Author

Just to verify that I share your vision, would the sketch below appeal to you/your idea?

image

I'd like to hear other opinions on your idea first but I could live with this implementation.
Having an additional one and a half sections (lan and history/last visited) would definitely add even more complexity to the list.

@tigercoding56
Copy link

nice, however could you move the buttons to stay on the blue background (maybe after the scroll bar) ,

@appgurueu
Copy link
Contributor

An alternative to the checkbox would maybe be having the user click/doubleclick the 'Last Visited' header to toggle it, turning the text grey when disabled or something

Personally I think collapsible sections would be fine, which is something table[] already supports - no need to implement it yourself. Note also (for future features) that these can be nested.

@siliconsniffer
Copy link
Contributor Author

I'm fine with either implementation, both seem to have their up- and downsides.

Thanks for pointing out the table[] tree option, forgot about that. I think if we should go with that it would make the most sense to only have this section be collapsible and leave the rest as is?

nice, however could you move the buttons to stay on the blue background (maybe after the scroll bar) ,

Can you elaborate on that? Do you mean putting them right of the server list?

@appgurueu
Copy link
Contributor

I think if we should go with that it would make the most sense to only have this section be collapsible and leave the rest as is?

I don't think that would be supported too well. I'd just make them all collapsible for simplicity and consistency. Collapsing incompatible servers certainly makes sense. Collapsing favorites or public servers doesn't seem very useful, yet not necessarily useless.

@cx384
Copy link
Contributor

cx384 commented Jan 11, 2025

I like siliconsniffer's sketch. My original vision was to put the buttons/tab selectors/dropdown field at the top of the server list, but putting them left to the list seems to be even better.

I prefer the tab over the table[] solution.
The lists are not strictly district. E.g. you would either have to display a server in the Public section and the Favorites section at the same time, or only in the Favorites section, like it is the case now. The same applies to the last visited section. (You will have to add a favorite indicator for servers in the Public section.)
The current situation of the Favorites section also messes up sorting, this becomes an even bigger problem when we add "sort by" features in the future.
However, it may still be good, to make the Incompatible Servers section collapsible, but keep it as part of the Public section, or add a checkbox for completely hiding incompatible servers.

Mizokuiam

This comment was marked as spam.

@siliconsniffer
Copy link
Contributor Author

siliconsniffer commented Jan 12, 2025

Thank you very much for the feedback. I just want to briefly point out that this still needs roadmap approval before we get into further details. It might also be useful to discuss when/how quickly we want this to be implemented.

Personally, I doubt that I will be able to pull this off alone before the 5.11 feature freeze; if anyone has the resources, I'd be happy to collaborate to make that possible.

Edit: FF now, guess I'll try for 5.12...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ✨ PRs that add or enhance a feature @ Mainmenu Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants