Add support for multi account login. #1200

wiscow · 2020-08-29T16:18:47Z

No description provided.

wiscow · 2020-08-29T16:29:43Z

src/Game/UI/Controls/StbTextBoxCombo.cs

+        internal void SetItems(string[] items)
+        {
+            _items = items;
+            if (_items.Length > 0 && _arrow == null)


Check for null reference here.

wiscow · 2020-08-29T16:33:29Z

src/Configuration/AccountManager.cs

+    {
+        public static List<Account> Accounts;
+
+        public static string[] GetAccountNames(string serverName)


should this include port number? I'm thinking this is probably overkill but i suppose it is possible that a given server may have different servers running on the same IP and point to different ports.

wiscow · 2020-08-29T16:34:06Z

src/Configuration/AccountManager.cs

+
+        private static string PathToAccountFile()
+        {
+            string path = FileSystemHelper.CreateFolderIfNotExists(CUOEnviroment.ExecutablePath, "Data", "Profiles");


Will need to adjust if the profile directory becomes custom according to: https://github.com/andreakarasho/ClassicUO/pull/1180

wiscow · 2020-08-29T16:36:26Z

src/Game/Scenes/LoginScene.cs

+                    if(CurrentLoginStep == LoginSteps.VerifyingAccount)
+                    {
+                        AccountManager.SaveAccount(Settings.GlobalSettings.IP, Settings.GlobalSettings.Username, Settings.GlobalSettings.Password, Settings.GlobalSettings.SaveAccount);
+                        if (!Settings.GlobalSettings.SaveAccount)


This is questionable as it appears to move away from the current behavior. I am just not sure if the current functionality to keep these fields populated if the user does not have the "Save Account" checkbox enabled and then go back to the login screen is desired. It seems like a potential current bug, but this may be original functionality. Could be easily removed.

wiscow · 2020-08-29T17:23:55Z

src/Game/UI/Gumps/Login/LoginGump.cs

            {
                X = offsetX,
                Y = offsetY,
                Width = 190,
                Height = 25,
            });

+            _textboxAccount.OnOptionSelected += (sender, e) =>


I was looking for a place to unhook this but I couldn't find a good spot. It would probably be a good idea for memory management.

I removed the anonymous function and make it named function. Overrode disposed, and unhooked it there.

deccer · 2020-08-29T19:00:12Z

src/Game/UI/Controls/StbTextBoxCombo.cs

+                string item = items[i];
+
+                if (item == null)
+                    item = string.Empty;


Please wrap one-liners with curly braces, whenever you stumble upon them.

heh, this was a copy pasta from ComboBox, normally VS would pick it up for me but i turned it off because i normally use tabs and i kept ending up with tabs everywhere. will fix.

deccer · 2020-08-29T19:00:33Z

src/Game/UI/Controls/StbTextBoxCombo.cs

+        private void Label_MouseUp(object sender, MouseEventArgs e)
+        {
+            if (e.Button == MouseButtonType.Left)
+                _setIndex((int)((Label)sender).Tag);


Here as well... see one-liners comment above

src/Configuration/AccountManager.cs

deccer · 2020-08-29T19:06:10Z

src/Configuration/AccountManager.cs

-                var accounts = LoadAccountsFromFile();
-                Accounts = accounts.Where(x => x.Server == serverName).ToList();
+                Load(serverName);
+                return Accounts?.Where(x => x.Server == serverName).Select(x => x.UserName).ToArray() ?? new string[] { };


I see this a lot as well, x instead of a proper name for what x might be. Why not account? And in case the line gets "too long", you can break the line before .Select ;)

bad habits die hard :)

src/Configuration/AccountManager.cs

deccer · 2020-08-29T19:28:55Z

src/Game/UI/Controls/StbTextBoxCombo.cs

+                    _contextMenu.OnItemSelected -= ItemSelectedHandler;
+                    UIManager.GetGump<ComboboxContextMenu>()?.Dispose();
+                    _contextMenu = null;


Can this be pulled out into a CleanupContextMenu()?

deccer · 2020-08-29T19:30:06Z

src/Game/UI/Controls/StbTextBoxCombo.cs

+            get => _selectedIndex;
+            set
+            {
+                _selectedIndex = value;


Maybe

if (_selectedIndex == value || value < 0) { return; } ...

Actually. This was also some copy pasta from the ComboBox and now that i look at it... None of this logic should really reside in this setter. This is probably better handled in the event handler for the OnItemSelected event that i added to the ComboboxContextMenu.

I have removed the auto property and moved all the relevant functionality to the event handler.

deccer · 2020-08-29T19:34:58Z

src/Game/UI/Controls/StbTextBoxCombo.cs

-				_arrow.Hue = 0;
-				TextboxComboContextMenu contextMenu = new TextboxComboContextMenu(_items, Width, 100, _font, selectedIndex => SelectedIndex = selectedIndex)
+				_arrow.Graphic = ARROW_UP;
+				if (x > _arrow.X && x < _arrow.X + _arrow.Width && y > _arrow.Y && y < _arrow.Y + _arrow.Height)


Hmm hmm hmm, I've seen this quite a bit, Maybe Gump should have a public method bool Contains(Point point).

I actually considered this. This is a pretty standard function for game development. I peeked in the Utilities folder to see if i could find anything, but no luck. I definitely agree on this.

Upon further investigation the GumpPic has its own event for mouse down and mouse up. Instead of doing the hitbox check i just subbed to the events on the GumpPic control itself and then the hitbox test is no longer needed.

subbed to the mouse events and moved the logic there. no more need for hitbox detection.

deccer · 2020-08-29T19:36:21Z

src/Game/UI/Gumps/Login/LoginGump.cs

+            // var savedAccounts = ProfileManager.GetAccounts(World.ServerName);
+            var savedAccounts = new string[] { "Hello World", "hello", "hello", "hello", "hello", "hello", "hello", "hello" };


Those two lines look bogus ;) Probably remains from testing.

I really should start merging my unit-test-branch. Then this can be covered by unit tests as well.

deccer · 2020-08-29T19:36:45Z

src/Game/UI/Gumps/Login/LoginGump.cs

+                // var savedPassword = ProfileManager.GetSavedPassword(World.ServerName, _textboxAccount.Text);
+                var savedPassword = "HelloWorld";


Also probably remains from testing around

andreakarasho · 2020-08-29T20:56:38Z

don't forget to pull updates from latest dev

deccer · 2020-08-30T12:04:04Z

src/Game/UI/Controls/StbTextBoxCombo.cs

+        public event EventHandler<int> OnOptionSelected;
+        public event EventHandler OnBeforeContextMenu;


Properties should go below the constructor.

deccer · 2020-08-30T12:05:09Z

src/Game/UI/Controls/StbTextBoxCombo.cs

+        {
+            _arrow.MouseDown -= ArrowMouseDownHandler;
+            _arrow.MouseUp -= ArrowMouseUpHandler;
+            _arrow = null;


_arrow.Dispose(); should be enough.

deccer · 2020-08-30T12:06:07Z

src/Game/UI/Controls/StbTextBoxCombo.cs

+        {
+            _contextMenu.OnItemSelected -= ItemSelectedHandler;
+            UIManager.GetGump<ComboboxContextMenu>()?.Dispose();
+            _contextMenu = null;


_contextMenu.Dispose(); should be enough here too

deccer · 2020-08-30T12:06:35Z

src/Game/UI/Controls/Combobox.cs

+            //Cleanup Context Menu
+            _contextMenu.OnItemSelected -= ItemSelectedHandler;
+            UIManager.GetGump<ComboboxContextMenu>()?.Dispose();
+            _contextMenu = null;


_contextMenu.Dispose(); should be enough

deccer · 2020-08-30T12:07:51Z

src/Configuration/Accounts.cs

+    {
+        public Account()
+        {
+


Sorry for being picky regarding formatting. Can you please remove the empty line here?
(SonarLint/Cube/StyleCop will cry about it, if we decide to add it :D)

deccer · 2020-08-30T12:09:05Z

src/Configuration/AccountManager.cs

+            if (existingRecord != null && !saveAccount)
+            {
+                _accounts.Remove(existingRecord);
+            }
+            //There is an existing record and they have updated their password.  save the udpated password.
+            else if (existingRecord != null && existingRecord.Password != password)
+            {
+                existingRecord.UpdatePassword(password);
+            }
+            //Otherwise if there is no existing record, the user has opted to save the account, and that account has a username save the record.
+            else if (existingRecord == null && saveAccount && !string.IsNullOrWhiteSpace(userName))
+            {
+                _accounts.Add(new Account(serverName, userName, password));
+            }


hmm hmm what about a switch block with pattern matching?

deccer

Just some minor changes I believe.

wiscow added 22 commits August 28, 2020 16:40

create stTextBoxCombo

aeac366

UI tweaks to component and add password update handler.

59d959d

move event invocation.

59fc188

placeholder for save account info.

eacc1a4

tabs to spaces.

2300692

tabs to spaces.

14ac8de

extract ComboboxContextMenu to an internal class.

ec16a66

remove need for ComboboxContextMenu to take in a Combobox.

67d9e82

remove duplicate context menu from stbTextBoxCombo.

3a4cf0c

Create account manager class.

9759d2f

wire up account manager.

c9c6c17

add vertical spacing param to context menu.

3c9f2c9

cleanup and exception handling.

1ed6d77

pass the save account setting into the acct manager.

0667c97

if user does not want account saved, clear out global settings.

8b00d6e

only save account info on VerifyAccount login step.

50202a4

remove scrolling test code.

aad2761

add input param for max combo height on new component.

5a715b2

remove un-needed constructor.

d297af7

remove testing code.

f58fed1

Merge branch 'dev' into multi-account-login

19667f5

order by Account Name.

cb4f92d

wiscow mentioned this pull request Aug 29, 2020

Request: Save Multiple Accounts in the config.json file and allow users to switch between accounts #1193

Open

wiscow commented Aug 29, 2020

View reviewed changes

use IP when checking acct passwords.

c6e4839

wiscow commented Aug 29, 2020

View reviewed changes

check for null on _items.

f1331a4

deccer reviewed Aug 29, 2020

View reviewed changes

deccer suggested changes Aug 29, 2020

View reviewed changes

wiscow added 9 commits August 29, 2020 16:02

AccountManager cleanup.

0f00b02

curly braces

d696d7a

extract CleanupContextMenu function.

0f0302d

Merge remote-tracking branch 'origin/dev' into multi-account-login

ae30717

code cleanup/review points.

e8ed06f

code cleanup/review points.

244935e

sub to the GumpPic mouse events for cleaner event handling.

dc5e440

remove SelectedIndex property and move code to event handler.

40fb934

dispose of account textbox event handler.

99cb27f

deccer reviewed Aug 30, 2020

View reviewed changes

deccer suggested changes Aug 30, 2020

View reviewed changes

wiscow added 2 commits August 30, 2020 12:03

code cleanup.

60e9769

Merge remote-tracking branch 'origin/dev' into multi-account-login

54e05df

andreakarasho changed the base branch from dev to main November 7, 2022 19:26

		// var savedAccounts = ProfileManager.GetAccounts(World.ServerName);
		var savedAccounts = new string[] { "Hello World", "hello", "hello", "hello", "hello", "hello", "hello", "hello" };

		// var savedPassword = ProfileManager.GetSavedPassword(World.ServerName, _textboxAccount.Text);
		var savedPassword = "HelloWorld";

		public event EventHandler<int> OnOptionSelected;
		public event EventHandler OnBeforeContextMenu;

Add support for multi account login. #1200

Are you sure you want to change the base?

Add support for multi account login. #1200

Conversation

wiscow commented Aug 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreakarasho commented Aug 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deccer left a comment

Choose a reason for hiding this comment