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

Provide UI feedback during Git command execution #630

Merged
merged 55 commits into from Jul 26, 2020

Conversation

kgryte
Copy link
Member

@kgryte kgryte commented May 1, 2020

Resolves #601 by providing UI feedback during Git command execution.

Overview

This PR

  • introduces an overlay over the JupyterLab window during the execution of the following Git UI actions:

    • refresh
    • commit
    • branch change
    • branch creation
    • reset
    • revert commit
  • cleans up the Git extension model by adding documentation, refactoring HTTP request handling to avoid error trapping, and instrumenting model actions to surface events which can then be shown in the JupyterLab status bar.

  • modifies error handling when creating a branch to allow displaying error messages directly in the new branch dialog.

  • adds two new extension settings to allow for customized extension behavior concerning UI suspension and status updates. This allows power users to configure extension behavior to their liking.

  • adds React components for displaying a blocking modal and toast notifications.

  • adds a status widget for displaying Git status updates in the JupyterLab status bar.

  • performs additional minor clean-up and documentation.

This PR addresses the fact that Git actions are executed asynchronously and such behavior can result in unexpected and undesired behavior.

For example, a user could attempt to push changes to a remote server. Currently, the UI only provides feedback upon receiving a response. If a user closes JupyterLab before receiving an error response, a user could walk away believing that her changes were successfully pushed. By blocking the UI and displaying a prominent status indicator, we encourage users to wait before further action, including closing JupyterLab.

Especially given the changes introduced in #564 where a server may not respond for several seconds while waiting for index.lock files to be removed, visual feedback indicating that actions are in-progress is critical in avoiding scenarios where work is lost (e.g., a commit fails unbeknown to the user, changes are overwritten upon pull) or conflicts arise (e.g., a merge action is submitted, changes are made, and a merge action resolves, resulting in conflicts).

For example, consider the scenario where a user attempts to commit changes. Suppose the command takes 20s due to Git hooks and the command fails. During interim, the user makes changes and initiates another commit. What happens? Currently, we simply move on to the next queued command. However, a user may want to have first resolved the hook failures (e.g., linting) before executing the next commit. In fact, sometimes, you want something in history (for posterity's sake), but then want to "undo" the change subsequently. So if I commit something I want in history, it fails, but my subsequent commit succeeds, now my history is messed up. In short, we simplify everything, and reduce extension complexity, by forcing sequential operations. Otherwise, a user needs to be able to interrupt a queue, canceling any pending operations after a failure.

By blocking the UI, we effectively force the user to slow down (albeit in minor, if any, manor and without much annoyance, depending on connection speed, etc) to allow commands to fully execute before making and initiating further changes and actions.

To be clear, forcing sequential operation is likely to not be a hindrance for local development or for users on fast networks.

Screenshots

  • Example overlay in light mode

ui_feedback_4_light_theme

  • Example overlay in dark mode

ui_feedback_5_dark_theme

  • Example upon encountering an error

ui_feedback_6_branch_creation_smaller

  • Example disabling UI suspension

ui_feedback_7_disable_suspend

Notes

Other UI feedback mechanisms considered:

  • JupyterLab status bar: a status bar indicator is easy to overlook and does not effectively dissuade a user from prematurely closing either the JupyterLab UI (browser tab) or the JupyterLab server before commands have finished executing. While implemented in this PR, using the status bar alone is not enough to effectively prevent users from performing actions leading to potential foot guns. Further, users are able to hide the status bar, and, thus, may never see messages indicating that the Git extension is currently working and the JupyterLab frontend should not be closed.
  • widget progress indicator: restricting the progress indicator to only the Git extension side panel and not the entire UI does not sufficiently guard against lost changes and conflicts. For example, if a user is still allowed to make file changes while commands execute (e.g., git pull), then, on pull, unsaved changes could be overwritten (and thus lost) or a merge conflict could arise which would not have arisen had the user waited until the pull was complete.

Demo

Here is a Binder link for the changes proposed in this PR: https://mybinder.org/v2/gh/kgryte/jupyterlab-git/ui-feedback?filepath=examples%2Fdemo.ipynb
.

@ianhi
Copy link
Collaborator

ianhi commented May 1, 2020

It's definitely an aesthetic improvement over the current git pull/push spinner which never rendered correctly for me! and I do like being prevented from inducing conflicts :)

A few things to consider:

  1. Can be flashy if the action is quick

If the action takes enough time to display the modal but is still completed quickly then it can make the screen kind of flashy. It's hard to capture in this gif because I had to up the frame rate to grab the right frames, which makes it a slow-mo gif when uploaded here.
flashy-high-framerate
Probably easier to see if you try yourself. I added this delay with await new Promise(resolve => setTimeout(resolve, 200)); Though, for everything I tried without adding an artificial delay this wasn't a problem.

  1. Minimize the number of situations where this is necessary

Agreed that blocking the whole screen is the only surefire way to prevent the user from messing with things during a git command. However, I don't love having my full screen taken over. Could the number of places where this happens be minimized? For example:

  • refresh: maybe not necessary here?
  • branch creation: there is already a full screen blocking dialog used to create the branch, this could be used to block instead of spawning the modal on top of that.
  • commit only needs to block the git panel. Modifying a file during an ongoing commit should be a safe thing to do.
  1. make it dismissable?

If something like a push is taking a long time it might be nice to include an option to dismiss the dialog similar to the current git push/pull ui that uses the jupyterlab/apputils Spinner. I'm imagining that I need to switch what I'm working on and I've done bad thing by committing several very large files so pushing will take awhile. Here not being allowed to switch folders to whatever else I want to work on is potentially frustrating.

  1. Use the status bar as well

For scenarios like the push you described, having a confirmation of success on the status bar could be nice in order to provide firmer confirmation of success than just the modal going away.

@fcollonval
Copy link
Member

@kgryte I like the animation but it should definitely not block the all application.

For all softwares I know that integrate VCS, none is blocking the UI when carrying out git commands. And this is clearly what I would expect from this extension (for instance I hate the current modal for pulling and pushing that block me on background synchronization tasks). Therefore the overlay should not expand outside of the Git panel. But then come the question, what if the panel is hidden. So I endorse too the idea of having feedback thanks to the status bar.

kgryte added 26 commits May 11, 2020 11:18
I am not convinced this is an entirely good idea, as why bother
with a blocking modal at all if a user can dismiss. The main hope
is that less technical users will simply respect the modal and wait
until completion before moving on. The risk is that users will
catch on and disregard the warning, allowing for potential footguns.
However, reviewers appear hostile to a blocking modal which cannot
be readily dismissed.
@ianhi
Copy link
Collaborator

ianhi commented Jun 1, 2020

@kgryte I tried it out and this PR causes me to be unable to commit:
image
If I switch to the master branch and use the same repo then I don't have this issue. I use SSH for git credentials if that matters.

Otherwise I've really come around to it. It looks nice and the removal of footguns is definitely positive.

2

yes, we want to show a progress indicator because, as a user, I want to know that everything is up-to-date before I continue editing

makes sense. This actually made me realize that I miss having any feedback when I refresh the jupyterlab filebrowser.

3
Thanks :)

4
As for the Status bar updates I've suddenly arrived at second thoughts. The changing every 3 seconds in the bottom left corner of vision is pretty distracting. With the nice toast notifications you made maybe this isn't necessary? Or perhaps refresh could be excluded from status bar updates. could we prevent refresh from triggering a status bar update?

@kgryte
Copy link
Member Author

kgryte commented Jun 1, 2020

@ianhi Thanks for trying it out! Will investigate the issues you mentioned above and circle back.

@ianhi
Copy link
Collaborator

ianhi commented Jun 1, 2020

One other thought that may have a bad hassle/benefit ratio:

Could the modal be made to display only after the command has been running for some time that is short for humans but long for computers? i.e:

  • Run command
  • wait 200ms
  • if command not finished
    • spawn modal for minimum of 1 second
  • continue as normal

The idea being to maintain protection against conflicting commands while reducing visual interruption. Though maybe this would just have the effect of making the extension seem laggy which I think would be worse.

@kgryte
Copy link
Member Author

kgryte commented Jun 8, 2020

@ianhi

  • Re: 1. Tracked down the commit bug. Should be fixed.

  • Re: 4. We could exclude refresh from the status bar; however, this is also "useful" information, as it provides feedback when changing the refreshInterval setting and clues in the user that "stuff" is happening (e.g., HTTP requests) which might be costly (e.g., AWS bill). A user might see the status bar updating and decide to decrease the refresh interval; however, they may not be aware that that will mean the UI becomes laggy (e.g., switching a branch might take a while to appear in the UI, especially if done outside of the JLab UI, such as at the command-line).

    If other reviewers object to including the refresh status updates, I can remove; but, for now, I lean toward keeping them in. This PR allows disabling Git extension status bar updates altogether via the displayStatus extension setting. So, if toasts are sufficient, then a user can just disable status updates.

  • Re: delayed modal onset. I think this will make the UI seem laggy, so I am not inclined to introduce such a modal lag.

@kgryte
Copy link
Member Author

kgryte commented Jun 8, 2020

For those interested, here is a Binder link for viewing the changes proposed in this PR: https://mybinder.org/v2/gh/kgryte/jupyterlab-git/ui-feedback?urlpath=lab/tree/examples/demo.ipynb
.

@kgryte
Copy link
Member Author

kgryte commented Jun 8, 2020

@telamonian Should be ready for review now that the commit bug is resolved.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @kgryte for this big, yet modular 😉, work.

I only have one concern with this change. Up-to now, the model has close to a service for git commands. But with this, this is definitely no more the case as it increases the link with the UI.
So it would be nice to extract a service for the git commands after this is merged.

As example of the need for a service, I can point to that repository: https://github.com/fcollonval/jupyter-project

src/model.ts Outdated Show resolved Hide resolved
src/model.ts Show resolved Hide resolved
@kgryte
Copy link
Member Author

kgryte commented Jun 15, 2020

@fcollonval Thanks for the review!

Re: model-UI coupling. I don't believe these are more coupled than before this PR. The PR simply adds "logging" which could be directed to a file, to a UI, or to anywhere. The model is still decoupled; the change introduced by this PR is simply to afford model consumers the opportunity to listen in on what the model is doing if they so choose. Here, the PR opts to take the information and bubble up to the end user, but this is not required.

In general, there has been a long standing request by our clients to provide more insight into what the Git extension is doing when performing various operations/tasks. This PR simply moves us a couple steps in that direction. :)

@kgryte
Copy link
Member Author

kgryte commented Jun 15, 2020

@fcollonval Should be good to go!

@kgryte
Copy link
Member Author

kgryte commented Jul 6, 2020

@telamonian @fcollonval Are we able to move forward with this PR? :)

@fcollonval fcollonval added this to the 0.21.0 milestone Jul 10, 2020
@fcollonval
Copy link
Member

I'm working on this one to rebase and merge. Sorry for the delay @kgryte.

fcollonval added a commit that referenced this pull request Jul 26, 2020
commit 1c2cb43
Author: Frederic Collonval <fcollonval@gmail.com>
Date:   Fri Jul 24 18:24:42 2020 +0200

    Post Merge branch 'master' corrections

commit 5ec36d7
Author: Frederic Collonval <fcollonval@gmail.com>
Date:   Fri Jul 24 17:41:07 2020 +0200

    Merge with master

commit ae71d8d
Author: Athan Reines <kgryte@gmail.com>
Date:   Sun Jun 14 23:30:28 2020 -0700

    Use finally blocks

commit 2cd0408
Author: Athan Reines <kgryte@gmail.com>
Date:   Sun Jun 14 23:21:23 2020 -0700

    Reorder properties and methods

commit 9f2d7ca
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 8 11:08:57 2020 -0700

    Fix response consumption bug

commit e5d86d7
Merge: cab4132 6169af0
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 8 10:33:38 2020 -0700

    Merge branch 'master' of https://github.com/jupyterlab/jupyterlab-git into ui-feedback

commit cab4132
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 1 03:30:55 2020 -0700

    Fix broken tests

commit c4f0813
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 1 03:30:02 2020 -0700

    Fix broken tests

commit c132ced
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 1 03:06:07 2020 -0700

    Add private annotation

commit 28be7d5
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 1 03:04:40 2020 -0700

    Reorder methods

commit f2d40d8
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 1 02:57:31 2020 -0700

    Refactor to support UI feedback

commit b4d5513
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 1 02:22:26 2020 -0700

    Fix broken test

commit ab68b01
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 1 02:13:36 2020 -0700

    Refactor to support toast alerts

commit 99d6766
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 1 02:00:47 2020 -0700

    Refactor to support toast alerts

commit b1d79d6
Merge: 3daf4ba 8e79eae
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 1 01:44:23 2020 -0700

    Merge branch 'master' of https://github.com/jupyterlab/jupyterlab-git into ui-feedback

commit 3daf4ba
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 1 01:33:04 2020 -0700

    Refactor to support log message alerts

commit 2d48dda
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon Jun 1 01:18:06 2020 -0700

    Rename file and refactor into smaller components to support toasts

commit d203738
Author: Athan Reines <kgryte@gmail.com>
Date:   Wed May 27 16:11:15 2020 -0700

    Refactor to use "toast" notifications

commit 1d5c364
Author: Athan Reines <kgryte@gmail.com>
Date:   Wed May 27 16:10:17 2020 -0700

    Add Material UI dep for displaying alert messages within toast notifications

commit e0e6a5e
Author: Athan Reines <kgryte@gmail.com>
Date:   Wed May 27 13:34:04 2020 -0700

    Add support for providing UI feedback when committing changes

commit bf5c52a
Author: Athan Reines <kgryte@gmail.com>
Date:   Wed May 27 10:42:28 2020 -0700

    Fix broken tests

commit 4d84996
Author: Athan Reines <kgryte@gmail.com>
Date:   Wed May 27 10:40:37 2020 -0700

    Add src documentation and rename variables to be in line with project conventions

commit 018444b
Author: Athan Reines <kgryte@gmail.com>
Date:   Tue May 26 17:57:08 2020 -0700

    Fix broken tests

commit 29b9917
Author: Athan Reines <kgryte@gmail.com>
Date:   Tue May 26 17:54:23 2020 -0700

    Add support for providing feedback when creating a new branch and refactor error handling

commit 126389f
Author: Athan Reines <kgryte@gmail.com>
Date:   Tue May 26 17:13:08 2020 -0700

    Provide UI feedback when switching branches

commit b1ec725
Author: Athan Reines <kgryte@gmail.com>
Date:   Tue May 26 16:01:19 2020 -0700

    Add backticks

commit fc2edb0
Merge: 62d88de c34abef
Author: Athan Reines <kgryte@gmail.com>
Date:   Tue May 26 15:55:28 2020 -0700

    Merge branch 'master' of https://github.com/jupyterlab/jupyterlab-git into ui-feedback

commit 62d88de
Author: Athan Reines <kgryte@gmail.com>
Date:   Tue May 26 15:41:25 2020 -0700

    Document function

commit 1596dee
Author: Athan Reines <kgryte@gmail.com>
Date:   Tue May 26 15:39:47 2020 -0700

    Remove console.log

commit 697bab8
Author: Athan Reines <kgryte@gmail.com>
Date:   Tue May 26 15:32:06 2020 -0700

    Add support for toggling display of status bar updates

commit 2b9da5e
Author: Athan Reines <kgryte@gmail.com>
Date:   Tue May 26 14:45:30 2020 -0700

    Add setting to toggle status bar updates

commit c51a449
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 22:00:56 2020 -0700

    Allow the user to dismiss the modal

    I am not convinced this is an entirely good idea, as why bother
    with a blocking modal at all if a user can dismiss. The main hope
    is that less technical users will simply respect the modal and wait
    until completion before moving on. The risk is that users will
    catch on and disregard the warning, allowing for potential footguns.
    However, reviewers appear hostile to a blocking modal which cannot
    be readily dismissed.

commit b0a1c44
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 21:48:45 2020 -0700

    Fix operation order bug

commit 062a2a1
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 21:40:06 2020 -0700

    Move status widget to separate file and add widget style

commit c34c33e
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 21:22:16 2020 -0700

    Update lockfile

commit 5576d73
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 21:20:55 2020 -0700

    Restore prior refresh status behavior when non-200 response

commit 6bdba11
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 21:06:44 2020 -0700

    Throttle status widget to prevent flashing updates

commit cf3d9ac
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 19:57:07 2020 -0700

    Update task names and map log events to status messages

commit 8d471f4
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 19:29:12 2020 -0700

    Fix task queue management

commit ccbfb70
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 18:29:02 2020 -0700

    Add lumino collections dependency

commit 2f155e5
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 18:28:25 2020 -0700

    Refactor and clean-up the extension model

commit d373478
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 11:26:12 2020 -0700

    Wire up model event logging

commit 3b3b915
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 11:12:57 2020 -0700

    Fix capitalization

commit 0169c2a
Merge: a579a42 e923a5b
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 00:51:45 2020 -0700

    Merge branch 'master' of https://github.com/jupyterlab/jupyterlab-git into ui-feedback

commit a579a42
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 00:50:59 2020 -0700

    Fix capitalization

commit 71442cb
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 00:48:38 2020 -0700

    Fix capitalization

commit f3e25ec
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 00:32:52 2020 -0700

    Fix broken tests

commit f58b51f
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 00:23:45 2020 -0700

    Toggle UI suspension based on plugin setting

commit 6a93b34
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 00:23:05 2020 -0700

    Update setting description

commit 1e36470
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 18 00:22:24 2020 -0700

    Update description

commit 4437e60
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 11 11:22:06 2020 -0700

    Document new setting

commit 1bace21
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 11 11:20:34 2020 -0700

    Document setting

commit da2ec0e
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 11 11:18:38 2020 -0700

    Add setting for toggling UI suspension

commit 4ced5a8
Author: Athan Reines <kgryte@gmail.com>
Date:   Mon May 4 14:50:46 2020 -0700

    Ensure a minimum duration

commit 06300ff
Author: Athan Reines <kgryte@gmail.com>
Date:   Thu Apr 30 16:50:32 2020 -0700

    Add UI feedback during toolbar actions
@fcollonval
Copy link
Member

Merged manually in dafc400

@fcollonval fcollonval self-requested a review July 26, 2020 13:33
@fcollonval fcollonval merged commit 4c3dbe2 into jupyterlab:master Jul 26, 2020
@kgryte
Copy link
Member Author

kgryte commented Jul 26, 2020

Thanks @fcollonval!

fcollonval added a commit to fcollonval/jupyterlab-git that referenced this pull request Jul 28, 2020
fcollonval added a commit that referenced this pull request Jul 28, 2020
…11.x

Backport PR #630 UI feedback of Git execution
@ianhi ianhi mentioned this pull request Aug 11, 2020
@ianhi ianhi mentioned this pull request Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide feedback on git command execution
3 participants