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

close diff window when changes to file are discarded #1132

Merged
merged 3 commits into from
Jun 17, 2022

Conversation

BoscoCHW
Copy link
Contributor

@BoscoCHW BoscoCHW commented Jun 11, 2022

@fcollonval
Implemented closing of diff window when the changes to file are discarded, but I copied a lot of codes from the gitFileDiff command

Fixes #911


Edited to link the issue

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch BoscoCHW/jupyterlab-git/close-diff-window

@fcollonval
Copy link
Member

Thanks @BoscoCHW

We may take a slightly different approach to avoid code duplication.

When creating the diff widget at https://github.com/jupyterlab/jupyterlab-git/blob/master/src/commandsAndMenu.tsx#L854, we could add a callback on gitModel.statusChanged just after that. If the file is not changed any more (you will need to introspect gitModel.status.files), we will call widget.dispose() to close it.

@BoscoCHW
Copy link
Contributor Author

BoscoCHW commented Jun 13, 2022

@fcollonval
I implemented it by adding a callback when in the gitShowDiff command, though, I'm not sure if the implementation is good.
I have two questions:

  1. What is the purpose for the "unmodified" status? Based on my observation, discarding changes will remove the file from gitModel.status.files
  2. Do we need to disconnect the slot/callback when the widget is closed? I'm not sure how to do it.

@fcollonval
Copy link
Member

I implemented it by adding a callback when in the gitShowDiff command.

The implementation looks good, thanks for changing it.

I have two questions:
1. What is the purpose for the "unmodified" status? Based on my observation, discarding changes will remove the file from gitModel.status.files

The unmodified status is set there. It is set only when triggering the file browser context menu because we want to display the menu to open the git history for a versioned file that is not currently modified.

But in this case, this is as you figure out. If the file is not listed in gitModel.status.files, it is unchanged. Hence you can close the diff widget.

2. Do we need to disconnect the slot/callback when the widget is closed? I'm not sure how to do it.

Theoretically, it is a good practice to do so. But the widget itself has a clean up code when disposed to remove any slot.

src/commandsAndMenu.tsx Outdated Show resolved Hide resolved
Remove console log

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
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 a lot @BoscoCHW

@fcollonval fcollonval merged commit c266fb3 into jupyterlab:master Jun 17, 2022
@welcome
Copy link

welcome bot commented Jun 17, 2022

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

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

Successfully merging this pull request may close these issues.

Discard Changes Should close open diff
2 participants