Skip to content

fix: Handle deletion of unsaved copied file in NotebookPanel#1557

Merged
mofojed merged 9 commits intodeephaven:mainfrom
georgecwan:1359-overflow-menu-options
Oct 13, 2023
Merged

fix: Handle deletion of unsaved copied file in NotebookPanel#1557
mofojed merged 9 commits intodeephaven:mainfrom
georgecwan:1359-overflow-menu-options

Conversation

@georgecwan
Copy link
Copy Markdown
Contributor

@georgecwan georgecwan commented Oct 3, 2023

  • Previously, duplicating a notebook file and then trying to delete it without saving it first will cause an error to occur
  • Added a fileExists function to FileUtils to handle this check
  • Added a getUniqueCopyFileName function to FileUtils
  • Behaviour of copying a file from NotebookPanel changed from opening an unsaved copy of the file to duplicating the file in FileStorage first
  • Related to Copy/Rename/Delete are missing from notebook menu options #1359

Testing Instructions:

  • Open a file in the notebook panel
  • Copy the opened file by using the 'Copy File' option in the overflow menu
  • Delete the file by using the overflow menu (No error should occur)
  • Repeat the same steps but copy the file by right clicking the file in the file explorer
  • Delete the copied file again, this time it should also disappear from the file explorer

@georgecwan georgecwan requested a review from mofojed October 3, 2023 14:32
@georgecwan georgecwan self-assigned this Oct 3, 2023
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 3, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (a273e64) 46.36% compared to head (8952885) 46.33%.
Report is 5 commits behind head on main.

❗ Current head 8952885 differs from pull request most recent head a599387. Consider uploading reports for the commit a599387 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1557      +/-   ##
==========================================
- Coverage   46.36%   46.33%   -0.03%     
==========================================
  Files         564      569       +5     
  Lines       35787    35925     +138     
  Branches     8959     9000      +41     
==========================================
+ Hits        16591    16647      +56     
- Misses      19144    19226      +82     
  Partials       52       52              
Flag Coverage Δ
unit 46.33% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...oard-core-plugins/src/panels/FileExplorerPanel.tsx 57.54% <0.00%> (+2.59%) ⬆️
packages/file-explorer/src/FileUtils.ts 82.00% <0.00%> (-6.18%) ⬇️
...ashboard-core-plugins/src/panels/NotebookPanel.tsx 2.22% <0.00%> (-0.03%) ⬇️

... and 38 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

There's still a case that can result in lost data:

  1. Have a file named foo.py
  2. In File Explorer, right-click the file and select "Copy File". A new file foo-copy.py is created.
  3. Open foo-copy.py, make some edits, then save and close it.
  4. Open the original foo.py notebook
  5. From the notebook menu, choose "Copy File". It will open a new tab with the name foo-copy.py, effectively masking the file from the same name already on disk.
  6. From the new tab with the name foo-copy.py, select Delete File. It will delete the foo-copy.py file from disk.

I think the fix for that is in handleCopy to loop when getting the copy file name until you know that file doesn't exist.

@georgecwan
Copy link
Copy Markdown
Contributor Author

georgecwan commented Oct 3, 2023

That wouldn't fix the problem since the act of copying the file doesn't save it (this was the behaviour before my change) so trying to copy the same file twice without saving either of them first will still have the same problem as neither of the copies exist in FileStorage yet. Is there a way to get all of files that NotebookPanel has open?

@mofojed
Copy link
Copy Markdown
Member

mofojed commented Oct 4, 2023

Right that's a good point; or you could copy it from the notebook panel, then copy it from the file explorer after.
Yes, there is an openFileMap defined in ConsolePlugin that keeps track of all the files that are open. You could use redux to set that for the current dashboard (similar to setDashboardConsoleSettings) and then use that when copying files...

@georgecwan georgecwan requested a review from mofojed October 10, 2023 15:52
Copy link
Copy Markdown
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Make sure we get @dsmmcken 's approval before merging, as this does change behaviour (since the file is created right on copy, instead of requiring an explicit save after). I think it's better overall (too many weird cases the other way).

Comment thread packages/dashboard-core-plugins/src/panels/FileExplorerPanel.tsx Outdated
Comment thread packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx Outdated
Comment thread packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx Outdated
Comment thread packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx
@mofojed mofojed merged commit 4021aac into deephaven:main Oct 13, 2023
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 13, 2023
@georgecwan georgecwan deleted the 1359-overflow-menu-options branch October 13, 2023 15:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants