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.

Sign up for GitHub

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

Jump to bottom

Client-side editing overhaul #1401

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Client-side editing overhaul #1401

wants to merge 13 commits into from

Conversation

isc-bsaviano
Copy link
Contributor

@isc-bsaviano isc-bsaviano commented Jul 15, 2024 •
edited
Loading

  • Fixes Support non-isfs virtual workspaces for client-side editing #1278.
  • Fixes Automatically sync all file events with the server that happen inside a client-side workspace folder #1454. Automatically sync all file changes, creations and deletions in client-side workspace folders with the connected server. This can be turned off using the new objectscript.syncLocalChanges setting. This new setting replaces the old objectscript.importOnSave setting, which is no longer needed. If objectscript.importOnSave was set to false, the extension will set objectscript.syncLocalChanges to false upon activation so no user migration is required.
  • Create an index of all Classes, MAC and INT routines, and Include files inside non-isfs workspace folders:
    • Create two maps: one with the document name as the key and an array of Uris as the value, and one in the reverse direction.
    • Use a FileSystemWatcher to update the index when files are created, deleted or changed on disk and an onDidChangeTextDocument handler to update the index when a file is changed within VS Code.
    • Delete a file on the server if the last Uri for it in a workspace folder was deleted.
    • DocumentContentProvider.getUri() will use the index to determine the Uri for a class or routine in a non-isfs folder. The export settings are now only used for non-classes and routines in folders with the file scheme.
    • WorkspaceSymbolProvider will only return results from classes that are in the client-side folder by using the index to determine all known classes.
  • Automatically show and hide the InterSystems Explorer and Projects Explorer views based on the folders in the workspace. InterSystems Explorer is only shown if there is at least one non-isfs(-readonly) workspace folder. Projects Explorer is only shown if there is at least one isfs(-readonly). Both are shown if no workspace folders are open. This formally ties the InterSystems Explorer to client-side editing workflows and Projects Explorer to server-side editing workflows. This replaces the objectscript.showExplorer setting.
  • Cache the list of Studio Abstract Document extensions that are supported for each server connection so we can properly import them from client-side folders. Importing abstract documents is now independent of the objectscript.export settings, except for DFI’s which still check the export settings to preserve the path splitting behavior added by Improve client-side DFI workflow #808. Any file within a workspace folder that has a supported abstract document extension will be imported with the last part of the path used as the server name (except for DFI’s that match the export settings).
  • Don’t allow creation of an editable server-side folder via the Add Server Namespace to Workspace… command if the current workspace already has a client-side folder in it. This is to prevent the blending of client-side and server-side workflows in a way that can lead to lost changes or overwritten edits.
  • Change the default value of the objectscript.explorer.alwaysShowServerCopy setting to true. The InterSystems Explorer should always show the server copy since the local copy can be opened from the files explorer.
  • Change the default value of the objectscript.autoAdjustName setting to false. Now that we have an index of the workspace, we no longer require that a document's name match the file path for the extensions to find it.
  • Remove the objectscript.export.noStorage setting. Storage is an integral part of the class definition, and we shouldn’t allow removing it.
  • Remove the objectscript.export.dontExportIfNoChanges setting. I don’t think that turning it on adds any benefit to the user.
  • Check if the file system is writable before exporting or creating a new file.
  • Don’t show server-side source control menu options in the InterSystems Explorer or Projects Explorer.
  • Show MultiValue routines in the InterSystems Explorer.
  • Hide .bpl and .dtl documents in the InterSystems Explorer since they can’t be opened or edited.
  • Create a single error handling method for consistent error reporting.
  • Remove unneeded type definitions.
  • Upgrade ancient dev dependencies.

@isc-bsaviano
Copy link
Contributor Author

isc-bsaviano commented Jul 15, 2024 •
edited
Loading

@gjsjohnmurray @isc-rsingh I've opened this as a draft since this is a giant PR that needs thorough review. I have a few design questions for you that I'd like to resolve before formally starting the review process and seeking feedback from other users:

  1. Should I just remove objectscript.explorer.alwaysShowServerCopy? Keeping the setting gives users who liked the previous behavior a way to retain it, but is that behavior desirable? I can’t think of a good reason why you’d want the InterSystems Explorer to show the local copies other than “that’s the way it is by default”.

I've decided to keep this setting for backwards compatibility.

  1. Should I remove the special DFI folder-splitting on export added by Improve client-side DFI workflow #808? Doing so would be backwards incompatible, and that behavior is almost three years old. However, I doubt anyone is using it and it would make sense to treat all Studio Abstract Documents equally.

I've decided to keep this behavior for backwards compatibility.

  1. Should the automatic deletion of local files on the server be guarded by a setting so users can disable it? Maybe objectscript.deleteOnFileDelete, modeled after objectscript.importOnSave?

This is covered by objectscript.syncLocalChanges.

@isc-bsaviano
Copy link
Contributor Author

@daimor I'd appreciate your feedback on this as a regular client-side editing user.

@isc-rsingh
Copy link
Member

Is this worth publicizing on DC for feedback before merging?

@isc-bsaviano
Copy link
Contributor Author

@isc-rsingh I've already done so: https://community.intersystems.com/post/do-you-use-client-side-editing-vs-code-if-so-we-want-your-feedback

This was referenced Oct 18, 2024
Closed
Closed
@isc-bsaviano isc-bsaviano mentioned this pull request Nov 8, 2024
Open
@isc-bsaviano isc-bsaviano mentioned this pull request Nov 25, 2024
Open
@isc-bsaviano
Copy link
Contributor Author

FYI after I get a little more user feedback I will be marking this as ready for review. I made the follow architecture changes in my last commit:

  • Removed our onDidSaveTextDocument handler in favor of using the new client-side file system watcher for saving changes. I added code in FileSystemProvider to actually save files.
  • Folded all explorer nodes into a single file to fix module resolution issues I was getting due to circular dependencies between the separate files.
  • I improved error handling inside checkConnection().

@isc-bsaviano isc-bsaviano mentioned this pull request Nov 27, 2024
Open
@isc-bsaviano isc-bsaviano marked this pull request as ready for review December 2, 2024 12:17
@isc-bsaviano
Copy link
Contributor Author

@isc-rsingh @gjsjohnmurray I've marked this as ready foe review since I suspect that it will take some time to review. My plan after this is merged is to release a version 3.0.0 including this and implementations for at least the other issues currently assigned to me. I would like to have that out in early 2025.

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

@gjsjohnmurray gjsjohnmurray Awaiting requested review from gjsjohnmurray

@isc-rsingh isc-rsingh Awaiting requested review from isc-rsingh

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@isc-bsaviano @isc-rsingh