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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow user to toggle inline comment margin #1568

Merged
merged 76 commits into from May 16, 2018

Conversation

Projects
None yet
4 participants
@jcansdale
Contributor

jcansdale commented Mar 28, 2018

What this PR does

  • New horizontal editor margin is hidden behind feature flag

    • image
  • Adds a margin to the editor window

    • image
  • Clicking on checkbox (or filename/comment bubble) toggles the comment margin

    • image
  • Clicking on view x comments navigates to PR file changes

    • image
  • If there are no PR file comments it will show view changes

    • image
  • If there are no PR file changes the margin won't appear

    • image
  • Right clicking on solution file with active PR branch shows View Changes in # PR button

    • image
  • Right clicking a diff view opened using View Changes in Solution slows the option Open File in Code View. This will open the diff pane in a new code view.
    image

    • This also works with the diff view opened using the default Team Explorer - Changes action Compare with Unmodified...
  • Clicking View Changes in # PR navigates to equivalent line on PR diff view

    • image
  • Clicking View Changes in # PR on file without changes in PR shows warning on status bar

    • image

Metrics

  • NumberOfPullRequestFileMarginToggleInlineCommentMargin - When the user toggles the checkbox to show/hide comment margin
    image

  • NumberOfPullRequestFileMarginViewChanges - When user clicks on view changes/view comments link
    image

  • NumberOfNavigateToPullRequestFileDiff - When user navigates from editor to PR file diff via the GitHub.GoToSolutionOrPRFile command
    image

  • NumberOfPRDetailsNavigateToEditor - When user navigates from PR file diff to editor via the GitHub.GoToSolutionOrPRFile command (NOTE: this counter existed before)

image

  • ExecuteToggleInlineCommentMarginCommand - When the GitHub.ToggleInlineCommentMargin command is executed (via the checkbox or executing command directly)
    image

Technical notes

  • When opening a comparison window, the DifferenceViewerOptions.ScrollToFirstDiffId global option will be temporarily set to false (see #1631). This lets me immediately set the equivalent caret position when navigating to the the diff view.

Todo

  • Add metrics
    • Add counter for Open File in Code View actions
    • Add counter for executing GoToSolutionOrPullRequestFileCommand
  • Make View File show the comment margin
  • Fix issue where diff view moves to first change. see:
    image

Issues

  • The comments margin doesn't always appear when the comment bubble icon is clicked
    • Same issue when comments margin is always on
  • The UI appears on all text view types (including diff view)

Initial thoughts

My initial impression is that this UI element would be a great opportunity to surface other important information about the current PR. For example:

  • The PR number could be shown there
  • How many (non-inline) comments comments there are and quick navigation to them
  • Show the files that are part of the PR and allow navigation to them
  • Navigate to any of the inline comments on the current PR file
  • Show if there are any unread comments on the PR and notify when they arrive

Lots of opportunities to show things there would be useful to the user and make this UI element more discoverable (e.g. showing the PR number would be handy for navigation and give context to the comment bubble).

image

Related UI

People will rarely see the following UI. They need to be using an Enterprise/Professional version of VS and be editing a non-source file. This is when editing our GitHub.VisualStudio.vsct file:

image

When the user clicks on one of the sections, a bubble pops up with a scrollable list of sub-items with an automatic preview of each.

image

We could do a similar thing, except with a list of inline comments in the file and a preview of each comment. When the user clicks on the comment they're interested it, it would navigate to the inline comment in the source.

Similarly we could have a list of PR review comments, with a preview and navigation to the GitHub PR review UI.

Lastly when the user clicks on the PR number, there could be a list of changed files in the PR and the number of inline comments they have displayed. Clicking in the file would navigate to the PR file diff or editor.

This would give context for the comment margin toggle switch (which I think is too minimal/subtle at the moment).

Tackle in another PR (#1671)

  • Make comments appear on View File (margin appears but no comments 馃槙)
  • Make comments appear on RHS View Changes in Solution

Fixes #1565

jcansdale added some commits Mar 27, 2018

Prototype toggle margin using GlyphMargin
This toggles the GlyphMargin on and off.
It will be changed to toggle the comments margin.
Add InlineCommentMarginEnabled editor option
This option controls InlineCommentMargin visibility.
Make InlineCommentMargin implement IWpfTextViewMargin
Move implementation out of GlyphMargin.
Pass IWpfTextView into GlyphMargin
No need to pass IWpfTextViewHost.
Renamed marginVisual to marginGrid.
Store IWpfTextView not IWpfTextViewHost
We're only really interested in the TextView.
Only enable toggle margin when changes in PR file
Moved async initialization out of CommentsMarginViewModel.
Add navigation to next inline comment
Show the PR file name on margin.
Make the PR file name and comment icon clickable
They are now part of the checkbox lable.
Add/remove tagger according to margin visibility
We don't want live taggers running and monitoring live files when margin isn't visible.
Un/subscribe to live file depening on margin enabled
Only monitor live file when InlineCommentMarginEnabled.

@jcansdale jcansdale changed the title from [Spike] Allow user to toggle inline comment margin to [wip] Allow user to toggle inline comment margin Apr 18, 2018

jcansdale added some commits Apr 18, 2018

Subscribe to live file depending on margin visibility
We only need live updates is the margin is actually visible, not simply if it is enabled.

jcansdale added some commits May 11, 2018

Add metrics to PullRequestFileMargin
Increment NumberOfPullRequestFileMarginToggleInlineCommentMargin when
checkbox is toggled.
Increment NumberOfPullRequestFileMarginViewChanges when view
changes/comments is clicked.
Allow navigation from editable diff to code view
This allows navigation from View Changes in Solution to Code View.
It also allows navigation from the default Team Explorer changes diff
view (Compare with Unmodified...) to code view.
Add NumberOfNavigateToCodeView metric
Add metric for navigating from editable diff (e.g. `View Changes in
Solution`) to code view.
ExecuteGoToSolutionOrPullRequestFileCommand metric
Add metric for number of times
ExecuteGoToSolutionOrPullRequestFileCommand is executed.
NumberOfPRDetailsNavigateToEditor next to related
Move NumberOfPRDetailsNavigateToEditor next to related metrics and
comment that it should be renamed to NumberOfNavigateToEditor.
@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented May 14, 2018

Hey @jcansdale here are a few things I noticed today:

  1. The command GitHub.ToggleInlineCommentMargin is not actually toggling the comment margin for me. Although every time I execute the command the metric: ExecuteToggleInlineCommentMarginCommand increments just fine.

  2. Some of the metrics don't seem to be incrementing correctly.

NumberOfNavigateToCodeView is always 0 even after I select the option
image 2

NumberOfPullRequestFileMarginViewChanges is always 0 even after I click on the view comments link.
39930471-b713f17c-5532-11e8-8d3b-8f186d52b073

This is a bit more vague, but I've noticed that some metrics aren't incrementing regularly for: NumberOfPRDetailsNavigateToEditor, NumberOfNavigateToPullRequestFileDiff, or NumberOfPullRequestFileMarginToggleInlineCommentMargin. Instead when I perform those actions it appears to increment ExecuteGoToSolutionOrPullRequestFileCommand instead.

metrics for viewing inline comments

@StanleyGoldman

This comment has been minimized.

Contributor

StanleyGoldman commented May 14, 2018

Should it locate me to the comment when i click the "view comments" in the task bar?

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented May 15, 2018

@StanleyGoldman,

Should it locate me to the comment when i click the "view comments" in the task bar?

Good point. It doesn't locate you on the comment, it simply navigates to show changes in the PR (that happens to have x number of comments). The use can then view an already open comment or navigate to the next/previous comment.

Maybe, view changes in PR (x comments) would be clearer?

@grokys

grokys approved these changes May 15, 2018

A few comments on the code, but this looks good for a beta feature. I find the toggle control slightly confusing at the moment, but I understand that this feature needs iterating on, so lets discuss that elsewhere.

// HACK: We need to wait here for the inline comment tags to initialize so we can find the next inline comment.
// There must be a better way of doing this.
await Task.Delay(1500);
RaiseWhenAvailable(Guids.CommandSetString, PkgCmdIDList.NextInlineCommentId, param);

This comment has been minimized.

@grokys

grokys May 15, 2018

Contributor

I thought this hack was no longer necessary with DifferenceViewerOptions.ScrollToFirstDiffId?

This comment has been minimized.

@jcansdale

jcansdale May 15, 2018

Contributor

So did I! 馃槩 It seems there's another issue where the NextInlineCommentId depends on the comment margin tags having been updated in order to navigate to the next comment. Before the tags have been refreshed, this command is unavailable and it would crash when attempting to execute the command.

We should attempt to fix this in another PR.

{
[Export(typeof(EditorOptionDefinition))]
[Name(OptionName)]
public class InlineCommentMarginEnabled : ViewOptionDefinition<bool>

This comment has been minimized.

@grokys

grokys May 15, 2018

Contributor

I've never used this API, so I'm missing something here, but the only thing that seems to be used from this class is OptionName. Do we need to create a subclass of ViewOptionDefinition if we only use the name? Why not use an instance of this class as they key when getting options?

This comment has been minimized.

@jcansdale

jcansdale May 15, 2018

Contributor

By exporting ViewOptionDefinition we declare than a new options exists on the text views (and give it a default value). I probably shouldn't be using OptionName directly and should be importing the option and using the Key property. This gives us type information for the property.

I've changed OptionName to private and am using Key in the latest commit.

@@ -118,7 +118,8 @@
Text="{Binding Body, UpdateSourceTrigger=PropertyChanged}"
TextWrapping="Wrap"
VerticalAlignment="Center"
GotFocus="ReplyPlaceholder_GotFocus">
GotFocus="ReplyPlaceholder_GotFocus"
SpellCheck.IsEnabled="True">

This comment has been minimized.

@grokys

grokys May 15, 2018

Contributor

Wasn't there another PR with this feature?

This comment has been minimized.

@jcansdale

jcansdale May 15, 2018

Contributor

Well, there will be. I didn't try to sneak this one in, seriusly! ;-)

namespace GitHub.Commands
{
[Export(typeof(IGoToSolutionOrPullRequestFileCommand))]
public class GoToSolutionOrPullRequestFileCommand : VsCommand, IGoToSolutionOrPullRequestFileCommand

This comment has been minimized.

@grokys

grokys May 15, 2018

Contributor

Could we have a comment here (and on the interface) as it's not totally clear from the class name what this command will do.

This comment has been minimized.

@jcansdale

jcansdale May 15, 2018

Contributor

Done. I hope it make sense. Let me know if it isn't clear.

jcansdale added some commits May 15, 2018

Use InlineCommentMarginEnabled.Key not OptionName
By using Key we can take advantage of the option's type information.
Import Lazy<IPullRequestSessionManager> directly
No need to go through IGitHubServiceProvider.
Store keys in InlineCommentTextViewOptions
Use InlineCommentTextViewOptions for typed editor option keys.
@jcansdale

This comment has been minimized.

Contributor

jcansdale commented May 15, 2018

@meaghanlewis,

Some of the metrics don't seem to be incrementing correctly.

Thanks for the heads up! I don't understand why this is happening. 馃槙 I'll investigate more tomorrow.

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented May 16, 2018

@meaghanlewis,

The command GitHub.ToggleInlineCommentMargin is not actually toggling the comment margin for me. Although every time I execute the command the metric: ExecuteToggleInlineCommentMarginCommand increments just fine.

The GitHub.ToggleInlineCommentMargin command toggles an option on the current editor that determines whether the comment margin is visible in certain circumstances (the user is on a PR branch and editing a file that has changed in the PR).

When you execute GitHub.ToggleInlineCommentMargin from the Command Window, it will toggle this option on the command window itself. If you were to bind the command to a keyboard shortcut, you should find it works as expected (I've bound it to Alt+G, Alt+T).

@jcansdale jcansdale merged commit 6cf4a55 into master May 16, 2018

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jcansdale jcansdale deleted the fixes/1565-comments-margin-toggle branch May 16, 2018

@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented May 17, 2018

@jcansdale it's still hard for me to understand what GitHub.ToggleInlineCommentMargin. I don't see it have any affect on whether the comment margin is visible or not when viewing a changed file on a PR (with the branch checked out). Could you show me perhaps?

@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented May 17, 2018

Also, @sguthals and I were noticing that when enabling or disabling the feature flag, that a file (or files) need to be closed and reopened to actually see the options to toggle the inline comment margin. This is probably to be expected, and I'm sure users won't just be toggling the flag often, but just wanted to bring it up.

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented May 21, 2018

@meaghanlewis,

it's still hard for me to understand what GitHub.ToggleInlineCommentMargin. I don't see it have any affect on whether the comment margin is visible or not when viewing a changed file on a PR (with the branch checked out).

I think the confusion might be that it's a per-editor view setting, not a global show comments in the margin for all file setting. It is triggered when you click on the check box (when the horizontal margin is visible).

image

I made it that way because the horizontal margin is constructed per-editor view. It might make more sense for it to be a global show the editor margin setting. The checkbox would remain set when moving between PR files and executing GitHub.ToggleInlineCommentMargin would toggle the checkbox on and off. I think that would be a lot less confusing!

Also, @sguthals and I were noticing that when enabling or disabling the feature flag, that a file (or files) need to be closed and reopened to actually see the options to toggle the inline comment margin. This is probably to be expected, and I'm sure users won't just be toggling the flag often, but just wanted to bring it up.

Yes, that is by design. To make it dynamic, the margin would need to be loaded but hidden. I've made it so the switch completely disables this functionality. I got bitten by this once before when hidden functionality was causing issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment