raduprv / Eternal-Lands

http://www.eternal-lands.com
Other
158 stars 57 forks source link

First working prototype of server chosing option in login screen #93

Open brunoramoslu opened 3 years ago

brunoramoslu commented 3 years ago

This is a first version of possible implementation for having a way to chose the server in the login screen. (resolves #38)

I would like some feedback if the direction I'm going is ok or not.

For the moment the server selection is done by entering the id of the entry in the servers.lst file.

The server selection would need to be updated to a combo box.

And it would be nice to be able to edit the servers.lst entries from the interface too.

To test this I deleted a my .elc folder, and I copied a version of the servers.lst and the el.ini files to the base folder of the code.

I was able to test successfully:

pjbroad commented 3 years ago

Thanks for the pull request, this feature has been requested for a long time! Currently, the client finds the servers.lst file in the users config base directory, and then uses the specified server ID (main by default) to set the config subdirectory. It then loads the el.cnf and el.ini from there. The el.ini holds the location of the data directory. All these locations default to the working directory if files don't exist where expected.

If I understand your code correctly, you no longer use the config subdirectories which currently relate to the chosen server. Instead the server.lst and el.ini (assuming el.cnf too) are loaded from a fixed location. This fixed location is the current working directory though they are also found if located in the users base config directory ($HOME/.elc on Linux). After loading servers.lst, el.cnf and el.ini from these fixed locations, we just need the server name to connect to, which can now be entered by the user.

It certainly works. I agree a selection list would be nice for the server input.

The issue I see is that people like having the config subdirectories. If there was a way to select between the old and this new approach, I think this approach would be fine. It would probably have to default to the old approach for existing users but your new approach could potentially be made the default for new installs.

Your new approach also does not allow reading any server message before login, but that may be OK.

It looks like the el.ini and el.cfg files currently get written back to the config subdirectories on client exist which I guess is a bug. I also had a client crash when testing, after entering the server name and attempting login. When I ran using GDB to find why, it stopped happening of course; I'll let you know when I find it.

brunoramoslu commented 3 years ago

Thanks for the pull request, this feature has been requested for a long time! Currently, the client finds the servers.lst file in the users config base directory, and then uses the specified server ID (main by default) to set the config subdirectory. It then loads the el.cnf and el.ini from there. The el.ini holds the location of the data directory. All these locations default to the working directory if files don't exist where expected.

Thanks for this, I struggled a little bit to understand that on my own. It's good to have confirmation of the current behaviour.

If I understand your code correctly, you no longer use the config subdirectories which currently relate to the chosen server. Instead the server.lst and el.ini (assuming el.cnf too) are loaded from a fixed location. This fixed location is the current working directory though they are also found if located in the users base config directory ($HOME/.elc on Linux). After loading servers.lst, el.cnf and el.ini from these fixed locations, we just need the server name to connect to, which can now be entered by the user.

It certainly works. I agree a selection list would be nice for the server input.

Correct, this is the general idea.

With the addition that I load the correct server configuration before we login, even though the code is currently unable to apply the dedicated server config for the moment. This is done in the set_server_details_from_id function call.

The issue I see is that people like having the config subdirectories. If there was a way to select between the old and this new approach, I think this approach would be fine. It would probably have to default to the old approach for existing users but your new approach could potentially be made the default for new installs.

I'll try a couple of solutions for this:

I'm open to other suggestions.

Your new approach also does not allow reading any server message before login, but that may be OK.

I noticed that too, and I think we could come up with a way to show the server message in the login window, maybe by adding a text box to the screen. Once the server is known and before the login is done (return key, or login button), we start the connection and show the message there. Maybe even with an indicator red/green showing if the server is available or not.

It looks like the el.ini and el.cfg files currently get written back to the config subdirectories on client exist which I guess is a bug. I also had a client crash when testing, after entering the server name and attempting login. When I ran using GDB to find why, it stopped happening of course; I'll let you know when I find it.

Those two should be bugs, I need to take a closer look at that.

pjbroad commented 3 years ago

I wonder if there's a more simple way to do this that uses most of the code you have already written? How about we keep the current method as it is, all the way to the login screen, but with your server input box. The server defaults to the configured value as normal, you get the server messages as normal. However, if you change the value in the server input box, the client disconnects from the current server and connects to the new one. This keeps the current behaviour as is, but allows you to change the server. I've tried a simple hack to test this and it works fine.

brunoramoslu commented 3 years ago

Sounds good enough to me. We could give it a try.

I just need to convert the text box to a combo box.

pjbroad commented 3 years ago

Not that it's particularity useful, but here's my simple hack based on the current code, not your fork. Regardless of which server is specified when starting the client, this hack just changes the server to "main" before login. It uses the force_server_disconnect() function - which I hope will be of use to you.

index b44da405..e2941b50 100644
--- a/loginwin.c
+++ b/loginwin.c
@@ -16,6 +16,7 @@
 #include "password_manager.h"
 #include "rules.h"
 #include "sound.h"
+#include "servers.h"
 #include "tabs.h"
 #include "textures.h"
 #include "translate.h"
@@ -441,6 +442,7 @@ static int click_login_handler (window_info *win, int mx, int my, Uint32 flags)
                set_username(input_username_str);
                set_password(input_password_str);
                passmngr_destroy_window();
+               set_server_details();
                send_login_info ();
        }
        //check to see if we clicked on the ACTIVE New Char button
@@ -511,6 +513,7 @@ static int keypress_login_handler (window_info *win, int mx, int my, SDL_Keycode
                set_username(input_username_str);
                set_password(input_password_str);
                passmngr_destroy_window();
+               set_server_details();
                send_login_info();
                return 1;
        }
diff --git a/main.c b/main.c
index 521f62d5..472fa98d 100644
--- a/main.c
+++ b/main.c
@@ -425,6 +425,15 @@ void       read_command_line(void)
  */
 char * check_server_id_on_command_line()
 {
+       static int done_once = 0;
+
+       if (done_once)
+       {
+               force_server_disconnect("testing");
+               return "main";
+       }
+       done_once = 1;
+
        if (gargc < 2)
                return "";
brunoramoslu commented 3 years ago

I'll give it another try this week and report back,

brunoramoslu commented 3 years ago

Well I was playing around with something for this...

I'm trying to have the following behaviour:

When the client starts you are not connected automatically to a server. When you click Login the first time it connects you to the server and shows the "Hello" message from the server. If you click Login again without changing the server text box the client performs the login action and everything that follows.

If you change the text box after the first login (when only connect to server), it disconnects you from the server with a message.

image

The design might need a bit of work :slightly_smiling_face: The code is so ugly at the moment I cannot commit it, I need to clean it up a bit before having an acceptable proof of concept.

And the remaining of the items discussed are on my TODO...

pjbroad commented 3 years ago

I don't know how close you are to completing this, but I wonder if rather than have the console on the login screen, the client could just show the existing console again if you change server. Then on pressing a key, return to the login screen again. This then behaves the same as first starting the client.

The other thing I wondered about was the issues people will get if they use the same character name on different servers. The client will not be able to use separate files for the two characters. We would have to tag the files with the server name too.

The separate server directory works well. I just wonder if we can make this work well enough.

gvissers commented 3 years ago

The other thing I wondered about was the issues people will get if they use the same character name on different servers. The client will not be able to use separate files for the two characters. We would have to tag the files with the server name too.

The separate server directory works well. I just wonder if we can make this work well enough.

After spending last night concocting plan after plan, each more elaborate and ridiculous than the previous one, my conclusion is that there's no way that that's going to work. Since currently the server is tied to the profile, your el.ini (which specifies interesting things like e.g. the window size with which the client should be created, or your data directory) automatically implies a server. Strictly speaking you could not even open a window right now or load any graphics without selecting a server. And while I realize I may be in the minority here, I do use different data directories for different profiles.

The only solution I see, is to decouple the server from the profile. We could create a new profiles.lst (or whatever you would want to call it), with fields profile ID, profile directory, and perhaps default server ID and/or allowed server IDs, and remove the Config dir column from servers.lst. When starting the client, you can select a profile ID which gives you an el.ini and data directory, but you don't connect to a server until you choose one in the server selection screen and hit login (or new character).

Password manager files are still coupled to a server profile, so those should be toggled when changing the server. We would need some place to store whether the password manager is enabled, as that also depends on the server.

We currently have the profiles stored directly in the config directory. Perhaps we want to create another layer, storing all profile directories in config_dir/profiles, and server directories (with password files) in config_dir/servers.

Does this sound reasonable? Obviously we would need some kind of update scheme for existing profiles, but it's the only way I could think of that would let us have both profiles and in-game server selection.

pjbroad commented 3 years ago

Does this sound reasonable? Obviously we would need some kind of update scheme for existing profiles, but it's the only way I could think of that would let us have both profiles and in-game server selection.

It all sounds a little complex. I feel sorry that @brunoramoslu has spent time on this but perhaps we just stick with what we have. Years ago on Linux, I created a simple launcher app that gets run before the client and offers selection from the configs available in the servers.lst file. Personally, I normally only use main and testing and these are available as right-click options from the desktop icon I use (part of the packages I make).

Perhaps we could consider a very simple cross-platform app that does the job of the above launcher script?

gvissers commented 3 years ago

It all sounds a little complex.

Hmm, it made sense in my mind. Then again, I was very sleep-deprived at the time :laughing:

Personally, I normally only use main and testing and these are available as right-click options from the desktop icon I use (part of the packages I make).

I always launch from the command line, so I personally have no need for it either. But it seems to be something that people have wished for for a long time.

Perhaps we could consider a very simple cross-platform app that does the job of the above launcher script?

Possibly. It would probably be simpler, not sure if it'd be less work. Ah well, I was going to shelve this until after the next release anyway.

brunoramoslu commented 3 years ago

Hey, I have no problem with us "throwing this out the door" for another solution. I've put some work on it because I wanted to have something like this for awhile. It's perfectly fine to think of a new/better solution. I'll try to help as I can, but I'm kind of busy with personal life at the moment.🙂

brunoramoslu commented 3 years ago

I'm back from holidays, I'm reviewing the code again vs the latest SSL changes and the latest discussions.

If you start something on the standalone app let me know. :slightly_smiling_face:

I might indeed abandon this if do not find a "nice and proper" way of integrating these changes to the current code and behavior...