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 unhighlight on disconnect #67

Merged
merged 6 commits into from
Mar 1, 2019
Merged

Conversation

parithon
Copy link
Collaborator

Purpose

When streamers disconnect from the chat service it would be helpful if the highlights that were generated while on stream automatically go unhighlight.

Files Changed

  • extension.ts: added the ability to automatically unhighlight when disconnecting from the chat service when the user sets the unhighlightOnDisconnect setting.
  • package.json: added the unhighlightOnDisconnect setting to the configuration section.

How to Test

  1. Clone the repo: git clone https://github.com/clarkio/vscode-twitch-highlighter.git
  2. Install the node dependencies: cd vscode-twitch-highlighter && npm install
  3. Press F5 or click debug button (green triangle/play button) in VSCode
  4. Set the Twitch Highlighter Unhighlight On Disconnect in your workspace settings
  5. Create a couple highlights while connected to the chat service
  6. Disconnect from the chat service

What to Validate

  • Make sure that when you disconnect from the chat service all highlights are removed.

Addresses

#64

@parithon parithon added the reviews wanted Generally a Pull Request with code changes that would be helpful to have validated by others label Feb 17, 2019
@clarkio clarkio added this to the 0.1.5 milestone Feb 22, 2019
Copy link

@NickLarsen NickLarsen left a comment

Choose a reason for hiding this comment

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

Working as advertised and good to go as it is.

From a user-friendliness perspective I might suggest a slightly different approach. The discoverability of this is not particularly kind; have a third option for "prompt when disconnecting" and enable it by default with an option in the popup to not make a choice and not ask again?

Other concerns, it's possible to disconnect unintentionally, for instance a service blip from twitch API or whatever, and losing all your highlights when you could easily reconnect is pretty crappy. If it's possible to identify when a disconnect happened for some reason other than user intentionally disconnecting, then prompt regardless, that would be a nice to have.

The user will be presented with a dialog that will ask what to do with the highlights when disconnecting from Twitch.

1) Always Remove - Sets the `unhighlightOnDisconnect` setting to true (will not display dialog again) and unhighlights all.
2) Remove - Unhighlights all.
3) Keep - Does nothing with the highlights.
@parithon
Copy link
Collaborator Author

parithon commented Feb 27, 2019

I decided to keep the unhighlightOnDisconnect setting to false by default. If the broadcaster disconnects from the chat, the unhighlightOnDisconnect is false, and highlights exist, a dialog will be displayed that will ask what to do with the highlights. Three options will be available:

  1. Always Remove - Removes all the highlighted lines and sets the unhighighlightOnDisconnect setting to true.
  2. Remove - Removes all the highlighted lines
  3. Keep - Does nothing, all highlights will remain and the chat will disconnect.

image

@clarkio
Copy link
Owner

clarkio commented Feb 27, 2019

@NickLarsen I tested locally and believe @parithon has addressed your valid concerns around the user-friendliness of this feature. When you get a moment could you re-review and let us know what you think.

Essentially now there is a notification to prompt you on how you want to handle the highlights on disconnect. Also if there is an interruption in the connection to the Twitch API that will not affect the highlights. The server implementation is set to attempt to reconnect as well.

Copy link
Owner

@clarkio clarkio left a comment

Choose a reason for hiding this comment

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

I'm going to approve for now so we can release and let people test that way too. If there is further feedback on this that would require some changes we can cross that bridge then. Thanks everyone for the help and input!

@clarkio clarkio merged commit 2f23462 into clarkio:vnext Mar 1, 2019
@parithon parithon deleted the feature-64 branch March 1, 2019 22:22
@NickLarsen
Copy link

Sorry for not getting back to this earlier. From the screenshot it looks like an improvement and keeping it off by default is fine for now.

CodemanCodes pushed a commit to CodemanCodes/vscode-twitch-highlighter that referenced this pull request Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviews wanted Generally a Pull Request with code changes that would be helpful to have validated by others
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants