Disable posting comments when logged out #23971

tybug · 2023-06-19T21:13:30Z

before	after	web

bdach · 2023-06-19T21:16:44Z

From the screenshots alone, the button should also change copy when logged out. You want CommentStrings.GuestButton{Reply,New} there (depending on context).

osu.Game/Overlays/Comments/CommentEditor.cs

tybug · 2023-06-19T21:16:49Z

osu.Game/Overlays/Comments/ReplyCommentEditor.cs

+            if (!TextBox.ReadOnly)
+                GetContainingInputManager().ChangeFocus(TextBox);


ChangeFocus(TextBox) adds a text cursor even when TextBox.ReadOnly == true. I'm not sure whether this is a framework-side bug and needs to be changed, but I've worked around it here.

It's prooobably a framework bug. Instinctual reaction says that text box shouldn't unconditionally accept focus but the impact of changing that to something like !IsReadOnly && !Disabled.Value would have to be tested.

tybug · 2023-06-19T21:22:32Z

From the screenshots alone, the button should also change copy when logged out. You want CommentStrings.GuestButton{Reply,New} there (depending on context).

The reason I held off on this is because those are actual buttons on web-side that activate the log-in popover. I wasn't sure how easy that would be to achieve osu-side, and I wasn't sure changing copy without also adding the corresponding functionality was appropriate.

I'll see if I can add this.

osu.Game/Overlays/Comments/CommentEditor.cs

bdach · 2023-06-19T21:28:46Z

The reason I held off on this is because those are actual buttons on web-side that activate the log-in popover. I wasn't sure how easy that would be to achieve osu-side

We do this elsewhere; see

osu/osu.Game/Screens/Menu/ButtonSystem.cs

Lines 163 to 183 in c467e6b

    
           private void onMultiplayer() 
        
           { 
        
               if (api.State.Value != APIState.Online) 
        
               { 
        
                   loginOverlay?.Show(); 
        
                   return; 
        
               } 
        
               OnMultiplayer?.Invoke(); 
        
           } 
        
           private void onPlaylists() 
        
           { 
        
               if (api.State.Value != APIState.Online) 
        
               { 
        
                   loginOverlay?.Show(); 
        
                   return; 
        
               } 
        
               OnPlaylists?.Invoke(); 
        
           }

Should be pretty easy but if it turns out to increase scope/size of changes too much then we can deal with that as a follow up. That shouldn't be the case, though.

Tests here would also be nice to see.

tybug · 2023-06-19T22:54:07Z

Have attempted to address all concerns here, including showing the login overlay. No tests yet - will work on that next.

One remaining concern is that the "sign in to comment" text is now too large:

I was hoping there would be a way to specify both a minimum width and autosize the X axis of the button drawable, which should match web behavior here. But I didn't even get as far as autosizing - the following patch results in the button having a size of 0x0 and disappearing from the layout, instead of autosizing to the text's width:

diff --git a/osu.Game/Overlays/Comments/CommentEditor.cs b/osu.Game/Overlays/Comments/CommentEditor.cs
index b139fbefe6..8e14a7920b 100644
--- a/osu.Game/Overlays/Comments/CommentEditor.cs
+++ b/osu.Game/Overlays/Comments/CommentEditor.cs
@@ -211,10 +211,11 @@ protected partial class EditorButton : RoundedButton
         {
             public EditorButton()
             {
-                Width = 80;
+                // Width = 80;
                 Height = 25;
                 Anchor = Anchor.CentreRight;
                 Origin = Anchor.CentreRight;
+                AutoSizeAxes = Axes.X;
             }
 
             protected override SpriteText CreateText()

Any idea what I'm doing wrong here?

peppy · 2023-06-20T08:56:54Z

Buttons don't support autosize to my knowledge, due to reasons.

Honestly I don't think the text on the button changing is a deal-breaker, I'd just leave it for a future effort.

If you do want to add it in this PR (and it hooks up to actually bringing the login overlay up) then I'd suggest making a separate button for it, rather than changing the text/behaviour of the existing one. Then choose which button to display based on context.

osu.Game/Overlays/Comments/CommentEditor.cs

tybug · 2023-06-21T06:34:55Z

I've split into two buttons as suggested, and added a test for logging in and out.

The test currently fails due to how DummyAPIAccess schedules things, though. Currently, DummyAPIAccess sets LocalUser before updating state:

osu/osu.Game/Online/API/DummyAPIAccess.cs

Lines 115 to 119 in aa96fef

    
           public void Logout() 
        
           { 
        
               LocalUser.Value = new GuestUser(); 
        
               state.Value = APIState.Offline; 
        
           }

Since CommentEditor binds to LocalUser and not State, logging out triggers an update but IsLoggedIn is true at that point in tests.

This is not an issue outside of tests because, if I'm reading this right, APIAccess explicitly sets LocalUser after any state bindables fire:

osu/osu.Game/Online/API/APIAccess.cs

Lines 498 to 505 in aa96fef

    
           // Scheduled prior to state change such that the state changed event is invoked with the correct user and their friends present 
        
           Schedule(() => 
        
           { 
        
               setLocalUser(createGuestUser()); 
        
               friends.Clear(); 
        
           }); 
        
           state.Value = APIState.Offline;

There's a similar but distinct issue with moving from offline to online. APIAccess defines IsLoggedIn => State.Value > APIState.Offline, but DummyAPIAccess defines IsLoggedIn => State.Value == APIState.Online. Note == vs >. This results in a discrepancy when State.Value == APIState.Connecting > APIState.Offline , which is the case when LocalUser updates in both APIAccess and DummyAPIAccess.

I propose the following patch to fix both of these issues:

diff --git a/osu.Game/Online/API/DummyAPIAccess.cs b/osu.Game/Online/API/DummyAPIAccess.cs
index 16afef8e30..421af93e6a 100644
--- a/osu.Game/Online/API/DummyAPIAccess.cs
+++ b/osu.Game/Online/API/DummyAPIAccess.cs
@@ -34,7 +34,7 @@ public partial class DummyAPIAccess : Component, IAPIProvider
 
         public string AccessToken => "token";
 
-        public bool IsLoggedIn => State.Value == APIState.Online;
+        public bool IsLoggedIn => State.Value > APIState.Offline;
 
         public string ProvidedUsername => LocalUser.Value.Username;
 
@@ -114,7 +114,13 @@ public void Login(string username, string password)
 
         public void Logout()
         {
-            LocalUser.Value = new GuestUser();
+            // important that LocalUser is updated after state for bindable events firing.
+            // Matches APIAccess.
+            Schedule(() =>
+            {
+                LocalUser.Value = new GuestUser();
+            });
+
             state.Value = APIState.Offline;
         }

Does the above look ok as a separate PR?

bdach · 2023-06-22T19:23:49Z

While it's unfortunate that we have to exercise correct ordering in DummyAPIAccess, the change above looks fairly innocuous and correct. I would however go for a bit of a tamer variant of it:

diff --git a/osu.Game/Online/API/DummyAPIAccess.cs b/osu.Game/Online/API/DummyAPIAccess.cs
index 16afef8e30..896fa30ff8 100644
--- a/osu.Game/Online/API/DummyAPIAccess.cs
+++ b/osu.Game/Online/API/DummyAPIAccess.cs
@@ -34,7 +34,7 @@ public partial class DummyAPIAccess : Component, IAPIProvider
 
         public string AccessToken => "token";
 
-        public bool IsLoggedIn => State.Value == APIState.Online;
+        public bool IsLoggedIn => State.Value > APIState.Offline;
 
         public string ProvidedUsername => LocalUser.Value.Username;
 
@@ -114,8 +114,8 @@ public void Login(string username, string password)
 
         public void Logout()
         {
-            LocalUser.Value = new GuestUser();
             state.Value = APIState.Offline;
+            LocalUser.Value = new GuestUser();
         }
 
         public IHubClientConnector GetHubConnector(string clientName, string endpoint, bool preferMessagePack) => null;

The stuff you mention about the LocalUser set being scheduled in the real APIAccess is all true, I just am not sure we need to simulate what that does to a T, and I am quite allergic to putting schedules in many places (tends to generate more bugs than it's worth). The only important thing is that the State value change callback sees the old user still.

As for whether it should be a separate PR - I don't know, feels almost like too much ceremony. I'd probably apply the above on master, run the whole test suite locally and see if there's anything else that needs fixing as a result of the change. If not, I'd just push that in here; however, if there are tests that would be broken by the above and would need their own adjustments, then indeed a separate PR is warranted.

wrt logging in and out

tybug · 2023-06-22T20:04:15Z

To be honest, I'm not sure why Scheduled is being used in APIAccess in the first place - as opposed to setting LocalUser and clearing friends after - so I'd agree with your patch as well.

It seems no tests are broken by the above patch, so I've applied it here.

peppy · 2023-06-23T04:17:55Z

To be honest, I'm not sure why Scheduled is being used in APIAccess in the first place - as opposed to setting LocalUser and clearing friends after - so I'd agree with your patch as well.

It's required for thread safety. Logout can be called from the api runner thread or any number of web requests callbacks (ie. on error), and we don't want that to trigger bindable event changes on a non-update thread.

peppy · 2023-06-23T04:25:46Z

osu.Game/Overlays/Comments/CommentEditor.cs

        }

        protected override void LoadComplete()
        {
            base.LoadComplete();
            Current.BindValueChanged(_ => updateCommitButtonState(), true);
+            User.BindValueChanged(_ => updateStateForLoggedIn(), true);


Is there a reason we're binding to User to get the api's State?

diff --git a/osu.Game/Overlays/Comments/CommentEditor.cs b/osu.Game/Overlays/Comments/CommentEditor.cs index 8dbafc0ea8..05bdac5966 100644 --- a/osu.Game/Overlays/Comments/CommentEditor.cs +++ b/osu.Game/Overlays/Comments/CommentEditor.cs @@ -14,7 +14,6 @@ using osu.Game.Graphics.UserInterface; using osu.Game.Graphics.UserInterfaceV2; using osu.Game.Online.API; -using osu.Game.Online.API.Requests.Responses; using osuTK; using osuTK.Graphics; @@ -36,14 +35,14 @@ public abstract partial class CommentEditor : CompositeDrawable protected TextBox TextBox { get; private set; } = null!; - protected readonly IBindable<APIUser> User = new Bindable<APIUser>(); - [Resolved] protected IAPIProvider API { get; private set; } = null!; [Resolved] private LoginOverlay? loginOverlay { get; set; } + private readonly IBindable<APIState> apiState = new Bindable<APIState>(); + /// <summary> /// Returns the text content of the main action button. /// When <paramref name="isLoggedIn"/> is <see langword="true"/>, the text will apply to a button that posts a comment. @@ -162,14 +161,14 @@ private void load(OverlayColourProvider colourProvider) }); TextBox.OnCommit += (_, _) => commitButton.TriggerClick(); - User.BindTo(API.LocalUser); + apiState.BindTo(API.State); } protected override void LoadComplete() { base.LoadComplete(); Current.BindValueChanged(_ => updateCommitButtonState(), true); - User.BindValueChanged(_ => updateStateForLoggedIn(), true); + apiState.BindValueChanged(updateStateForLoggedIn, true); } protected abstract void OnCommit(string text); @@ -177,12 +176,14 @@ protected override void LoadComplete() private void updateCommitButtonState() => commitButton.Enabled.Value = loadingSpinner.State.Value == Visibility.Hidden && !string.IsNullOrEmpty(Current.Value); - private void updateStateForLoggedIn() + private void updateStateForLoggedIn(ValueChangedEvent<APIState> state) { - TextBox.PlaceholderText = GetPlaceholderText(API.IsLoggedIn); - TextBox.ReadOnly = !API.IsLoggedIn; + bool isAvailable = state.NewValue > APIState.Offline; + + TextBox.PlaceholderText = GetPlaceholderText(isAvailable); + TextBox.ReadOnly = !isAvailable; - if (API.IsLoggedIn) + if (isAvailable) { commitButton.Show(); logInButton.Hide();

I didn't put too much thought into it. My thinking was that changes in User correspond to changes between just APIState.Offline and APIState.Online, but binding to state may fire for other state changes where the user hasn't actually changed: for instance, maybe your wifi going out could result in your api state changing APIState.Failing (I don't know whether this is actually how state works), resulting in an extraneous update.

Of course, since the ui only checks API.IsLoggedIn, no harm is done by firing the update function additional times for other states. It doesn't do any good, either, though.

If binding to state is generally considered better practice for checking IsLoggedIn then let's do that.

Yep, using state is the correct way. Feels weird to be using an arbitrary piece of information (User) to trigger an event then read the api state from a different field, after all.

disable posting comments when logged out

4a95430

pull-request-size bot added the size/S label Jun 19, 2023

bdach added the area:comments label Jun 19, 2023

tybug commented Jun 19, 2023

View reviewed changes

bdach changed the title ~~disable posting comments when logged out~~ Disable posting comments when logged out Jun 19, 2023

bdach added the type:online label Jun 19, 2023

bdach requested changes Jun 19, 2023

View reviewed changes

osu.Game/Overlays/Comments/CommentEditor.cs Outdated Show resolved Hide resolved

osu.Game/Overlays/Comments/CommentEditor.cs Outdated Show resolved Hide resolved

tybug added 3 commits June 19, 2023 17:36

resolve protected API in comments superclass

d5d494f

extract button text properties to methods, show login overlay on click

591277e

use runOnceImmediately instead of duplicating logic

f7dde53

pull-request-size bot added size/M and removed size/S labels Jun 19, 2023

tybug added 2 commits June 19, 2023 22:05

rename GetTextBoxPlaceholder to GetPlaceholderText

60eedba

update CommentEditor test components

3430524

bdach reviewed Jun 20, 2023

View reviewed changes

osu.Game/Overlays/Comments/CommentEditor.cs Outdated Show resolved Hide resolved

tybug added 4 commits June 21, 2023 00:37

move commitButton.Text update to appropriate method

8460873

use two separate buttons for posting / login

cc764af

fix cancel test for new button layout

dd4f271

add test for comment when logging in and out

6de7328

pull-request-size bot added size/L and removed size/M labels Jun 21, 2023

adjust DummyAPIAccess to more closely match APIAccess

21bed33

wrt logging in and out

bdach added 4 commits June 22, 2023 23:00

Rename and xmldoc members

786deec

Document why things were done in DummyAPIAccess

1672608

Rename test helpers

2c1c20e

Merge branch 'master' into comment-logged-out

c06b825

bdach previously approved these changes Jun 22, 2023

View reviewed changes

bdach requested a review from a team June 22, 2023 21:19

peppy self-requested a review June 23, 2023 04:15

peppy requested changes Jun 23, 2023

View reviewed changes

Bind to API.State instead of API.User

ce1579f

peppy dismissed bdach’s stale review via ce1579f June 23, 2023 05:05

Add Schedule to ensure correct thread for UI code

3432717

peppy approved these changes Jun 23, 2023

View reviewed changes

peppy enabled auto-merge June 23, 2023 05:08

peppy merged commit d864244 into ppy:master Jun 23, 2023

tybug deleted the comment-logged-out branch June 23, 2023 06:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable posting comments when logged out #23971

Disable posting comments when logged out #23971

tybug commented Jun 19, 2023

bdach commented Jun 19, 2023

tybug Jun 19, 2023

bdach Jun 19, 2023

tybug commented Jun 19, 2023

bdach commented Jun 19, 2023 •

edited

Loading

tybug commented Jun 19, 2023

peppy commented Jun 20, 2023 •

edited

Loading

tybug commented Jun 21, 2023 •

edited

Loading

bdach commented Jun 22, 2023 •

edited

Loading

tybug commented Jun 22, 2023

peppy commented Jun 23, 2023

peppy Jun 23, 2023

tybug Jun 23, 2023 •

edited

Loading

peppy Jun 23, 2023

		if (!TextBox.ReadOnly)
		GetContainingInputManager().ChangeFocus(TextBox);

Disable posting comments when logged out #23971

Disable posting comments when logged out #23971

Conversation

tybug commented Jun 19, 2023

bdach commented Jun 19, 2023

tybug Jun 19, 2023

Choose a reason for hiding this comment

bdach Jun 19, 2023

Choose a reason for hiding this comment

tybug commented Jun 19, 2023

bdach commented Jun 19, 2023 • edited Loading

tybug commented Jun 19, 2023

peppy commented Jun 20, 2023 • edited Loading

tybug commented Jun 21, 2023 • edited Loading

bdach commented Jun 22, 2023 • edited Loading

tybug commented Jun 22, 2023

peppy commented Jun 23, 2023

peppy Jun 23, 2023

Choose a reason for hiding this comment

tybug Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

peppy Jun 23, 2023

Choose a reason for hiding this comment

bdach commented Jun 19, 2023 •

edited

Loading

peppy commented Jun 20, 2023 •

edited

Loading

tybug commented Jun 21, 2023 •

edited

Loading

bdach commented Jun 22, 2023 •

edited

Loading

tybug Jun 23, 2023 •

edited

Loading