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

UI: Implement DeleteCookies for YT Service dock #10710

Closed

Conversation

msuman-google
Copy link

@msuman-google msuman-google commented May 22, 2024

Description

Implement DeleteCookies for YT service integration dock, similar to other service integration docks.

Motivation and Context

This will enable centralized cookie management for YouTube/Google account logins for other browser docks using the shared cookie manager. The existing YouTube Control Panel browser dock will be migrated to using the shared cookie manager (see #10747) so that sign-in cookies can be shared between YouTube Control Panel and YouTube Live Chat. This will unlock improved creator-facing experience for OBS users of YouTube Live Chat (such as better chat latency, creation end-points for polls, Q&A, moderation capabilities etc).

How Has This Been Tested?

Built OBS locally on Linux workstation, connected YouTube account (via OAuth), disconnected YouTube OAuth, verified no cookies were remaining. For purposes of testing, I copied over cookies from the YTCP cookie manager after logging into YouTube Studio.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@msuman-google msuman-google changed the title Implement DeleteCookies for the YT Service dock. UI: Implement DeleteCookies for YT Service dock. May 23, 2024
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Do not use merge commits to catch up on branch/repo history. Use git rebase.

Do not use final punctuation in PR titles or commit message subjects (the first line of a commit message).

@WizardCM WizardCM added the Enhancement Improvement to existing functionality label May 24, 2024
@msuman-google msuman-google changed the title UI: Implement DeleteCookies for YT Service dock. UI: Implement DeleteCookies for YT Service dock. May 24, 2024
@msuman-google msuman-google force-pushed the delete-cookies branch 2 times, most recently from 08657f3 to c00c7da Compare May 24, 2024 20:26
@msuman-google
Copy link
Author

Do not use merge commits to catch up on branch/repo history. Use git rebase.

Do not use final punctuation in PR titles or commit message subjects (the first line of a commit message).

Done.

@RytoEX RytoEX self-assigned this May 25, 2024
@msuman-google msuman-google requested a review from RytoEX May 28, 2024 18:00
@RytoEX RytoEX changed the title UI: Implement DeleteCookies for YT Service dock. UI: Implement DeleteCookies for YT Service dock May 28, 2024
@RytoEX
Copy link
Member

RytoEX commented May 28, 2024

Do not use final punctuation in PR titles or commit message subjects (the first line of a commit message).

This is still using final punctuation (a period at the end) of the commit message subject.

UI: Implement DeleteCookies for YT Service dock.

This will enable centralized cookie management for YouTube/Google
account logins for other browser docks using the shared cookie manager.

Could you elaborate on how this enables "centralized cookie management for YouTube/Google account logins"?

@msuman-google
Copy link
Author

Do not use final punctuation in PR titles or commit message subjects (the first line of a commit message).

This is still using final punctuation (a period at the end) of the commit message subject.

Done. Fixed, sorry, I did not realize that was disallowed.

UI: Implement DeleteCookies for YT Service dock.

This will enable centralized cookie management for YouTube/Google
account logins for other browser docks using the shared cookie manager.

Could you elaborate on how this enables "centralized cookie management for YouTube/Google account logins"?

Yes, see #10747. I'm working on another change to improve the chat UX when the user is signed-in, will send the pull request shortly.

@msuman-google msuman-google force-pushed the delete-cookies branch 2 times, most recently from 7ffed41 to 3c8a8b4 Compare May 29, 2024 16:43
@RytoEX
Copy link
Member

RytoEX commented May 30, 2024

Done. Fixed, sorry, I did not realize that was disallowed.

For what it's worth, our Contributing Guidelines cover this in the linked "How to Write a Git Commit Message".

Yes, see #10747. I'm working on another change to improve the chat UX when the user is signed-in, will send the pull request shortly.

Ideally, the context and "why" should be provided here, at least in summary form. If you must point to another PR for additional context, it should be linked in the Motivation section. In this case, this seems more like these changes "enable a future implementation of centralized cookie management" rather than enabling such itself, which was the main point of confusion internally while reviewing this. This by itself only "implements DeleteCookies", and the question brought up in review was, "why?" If you could answer "why?" in the PR's Motivation section, that would be helpful to reviewers.

@msuman-google
Copy link
Author

Yes, see #10747. I'm working on another change to improve the chat UX when the user is signed-in, will send the pull request shortly.

Ideally, the context and "why" should be provided here, at least in summary form. If you must point to another PR for additional context, it should be linked in the Motivation section. In this case, this seems more like these changes "enable a future implementation of centralized cookie management" rather than enabling such itself, which was the main point of confusion internally while reviewing this. This by itself only "implements DeleteCookies", and the question brought up in review was, "why?" If you could answer "why?" in the PR's Motivation section, that would be helpful to reviewers.

Done. Updated the motivation section.

@Warchamp7 Warchamp7 added the UI/UX Anything to do with changes or additions to UI/UX elements. label Jun 3, 2024
@msuman-google
Copy link
Author

Gentle ping for review, please LMK if the PR's motivation section looks better.

@msuman-google msuman-google force-pushed the delete-cookies branch 6 times, most recently from ff5af99 to 0dd305d Compare June 7, 2024 16:26
@msuman-google msuman-google force-pushed the delete-cookies branch 2 times, most recently from c136a53 to 289bc71 Compare June 17, 2024 17:21
@msuman-google msuman-google force-pushed the delete-cookies branch 2 times, most recently from 391c3c3 to 78e112d Compare June 26, 2024 23:54
@Warchamp7 Warchamp7 self-assigned this Jul 15, 2024
This will enable centralized cookie management for YouTube/Google
account logins for other browser docks using the shared cookie manager.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement to existing functionality UI/UX Anything to do with changes or additions to UI/UX elements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants