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

Update editor tab on member rename #1907

Conversation

steph-beneschan-256
Copy link
Contributor

@steph-beneschan-256 steph-beneschan-256 commented Mar 4, 2024

Changes

  • When the user renames a member from the object explorer, if the member was open in an editor tab, the tab will now be replaced by a tab showing the member with the new name.
  • If the user is currently editing a member, and there are unsaved changes, the user can no longer rename the member from the object explorer.

This is meant to provide a workaround to issue #1554 . It seems that the VS Code API does not let us directly change an open tab's label or uri, so instead I opted to have the system close the tab, then open a new one with the updated uri. Notably, the new tab will not necessarily be in the same position as the old one.

Hopefully this will suffice as a temporary solution, but I will continue to look into this.

Checklist

  • have tested my change
  • updated relevant documentation
  • Remove any/all console.logs I added
  • eslint is not complaining
  • have added myself to the contributors' list in CONTRIBUTING.md

@worksofliam
Copy link
Contributor

I will have a formal review in later for this, but should we consider implementing the same for stream files?

Perhaps the function you added to get the tab should be part of Tools?

@worksofliam worksofliam self-requested a review March 5, 2024 00:34
* The findMemberTab function is now defined in Tools.js as a function of the Tools namespace
@steph-beneschan-256 steph-beneschan-256 marked this pull request as ready for review March 5, 2024 04:24
@worksofliam
Copy link
Contributor

@steph-beneschan-256

Two things:

  1. Thanks for moving that API into tools. Are you also planning to add the same safeguarding logic for IFS objects too? E.g. when move/rename is used but the stream file is open, cancel the operation? This would be in ifsBrowser.ts. No is ok, just checking.
  2. Please fix the conflicts and I will test further!

@steph-beneschan-256
Copy link
Contributor Author

I will add the check to the IFS browser, and also resolve the conflicts.

* The IFS browser no longer allows the user to rename/move a file if the user is currently editing the file and there are unsaved changes
* When the user renames or moves an IFS file, all tabs where the file was open are now closed and reopened with the new file path

* Rewrote function findMemberTab as findUriTabs; the function now accepts URIs for members or streamfiles, and returns all tabs in which the resource is open (instead of just one)
* Added helper function areEquivalentUris to check whether two URIs (both assumed to represent members or streamfiles) are equivalent
@steph-beneschan-256
Copy link
Contributor Author

I made a few more commits; the new logic should now apply to the IFS browser as well as the object browser.

Also, I noticed that if I use the IFS browser to rename/move an IFS file to a new path, if an IFS file already exists at the new path, it will be overwritten. Is that the intended behavior? (I ask because if I try to do the same thing using the IFS utility in ACS, the operation fails and no files are modified.) If it's a problem, I wouldn't mind looking into it.

@worksofliam
Copy link
Contributor

@chrjorgensen What do you you think about @steph-beneschan-256's finding of the file being overwritten?

@chrjorgensen
Copy link
Collaborator

@chrjorgensen What do you you think about @steph-beneschan-256's finding of the file being overwritten?

@worksofliam It has always been like that - since the command executed on the server is a mv command which overwrites an existing file, if the existing file is writable. See mv documentation here: https://www.gnu.org/software/coreutils/manual/html_node/mv-invocation.html#mv-invocation

I think our users has gotten used to this behavior, we don't have any issues about this...

@worksofliam worksofliam requested review from a team and sebjulliand March 18, 2024 14:29
@worksofliam worksofliam linked an issue May 15, 2024 that may be closed by this pull request
@@ -127,6 +127,8 @@
"enddbgsvr.succeeded": "Serveur de débogage arrêté.",
"error": "erreur",
"errors": "erreurs",
"file.path.not.parsed": "The file path could not be parsed.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sebjulliand French - pleeeease! 😃

Copy link
Collaborator

@chrjorgensen chrjorgensen left a comment

Choose a reason for hiding this comment

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

@steph-beneschan-256 I tested this PR and it worked great. 👍

I also changed the texts into localized strings and translated into Danish.
@sebjulliand Could you translate the French texts and make the final approvement?

chrjorgensen and others added 3 commits May 28, 2024 17:54
Co-authored-by: Sébastien Julliand <[email protected]>
Co-authored-by: Sébastien Julliand <[email protected]>
Co-authored-by: Sébastien Julliand <[email protected]>
@sebjulliand sebjulliand self-requested a review May 28, 2024 15:58
Copy link
Collaborator

@sebjulliand sebjulliand left a comment

Choose a reason for hiding this comment

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

Approved!

@chrjorgensen chrjorgensen merged commit c27fdea into codefori:master May 28, 2024
1 check passed
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.

Object browser rename, does not "reload file's name"
4 participants