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

Cleanup/connection_manager_api #1975

Merged
merged 18 commits into from
May 14, 2024
Merged

Conversation

worksofliam
Copy link
Contributor

@worksofliam worksofliam commented Apr 4, 2024

Changes

Replaces raw access of connections setting and password context with a ConnectionManager namespace.

How to test this PR

  1. Ensure the same test cases pass
  2. Try out the extension as normal
  3. Confirm debug still works
  4. When editing connection settings or login settings, ensure the sort button is disabled.

Checklist

  • have tested my change
  • have created one or more test cases
  • updated relevant documentation
  • Remove any/all console.logs I added
  • have added myself to the contributors' list in CONTRIBUTING.md

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.

Nice cleanup @worksofliam ! Way cleaner than the raw accesses we used to do. I love it!
See the few comments I made, mostly code cleanup.

One thing to consider: since we update by index, there is a use case where the data can be saved on the wrong index:

  • Open the settings of a connection and keep it opened
  • Sort the connections browser
  • Save the opened settings

Since the indexes were changed while sorting, the index kept by the settings editor is not the right one anymore.
I admit it's an edge case...but if I thought of it, someone else will.
So maybe we should set a context value while the webview is opened to prevent the sort and rename actions from being executed.

@worksofliam worksofliam self-assigned this Apr 22, 2024
@worksofliam
Copy link
Contributor Author

@sebjulliand Let me look into how to solve this problem. Should have a fix for it by the end of the week. Thanks for your review!

@worksofliam worksofliam added this to the 2.11.0 milestone Apr 25, 2024
@worksofliam worksofliam requested a review from sebjulliand May 7, 2024 15:16
@worksofliam
Copy link
Contributor Author

@sebjulliand Sorry this took SO long. I went ahead and used withContext around the UI logic to disable the buttons. Check it out and let me know - thanks!

@worksofliam worksofliam mentioned this pull request May 7, 2024
10 tasks
@worksofliam worksofliam requested a review from sebjulliand May 13, 2024 20:27
@sebjulliand
Copy link
Collaborator

@worksofliam I'll fix a small issue here with withContext; in the meantime, how about adding that brilliant use of it to disable the buttons and disable code-for-ibmi.connectTo, code-for-ibmi.connectToAndReload, code-for-ibmi.renameConnection, code-for-ibmi.deleteConnection as well? WDYT?

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.

@worksofliam works like a charm! Brilliant cleanup!
Just see my last comment about disabling more commands to avoid potential issues and then we'll be good to go!

Copy link
Contributor Author

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

@sebjulliand I went ahead with your suggestion, and some other commands also. Thanks!

@worksofliam worksofliam requested a review from sebjulliand May 13, 2024 20:59
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.

Not sure about disabling "code-for-ibmi.showAdditionalSettings" and "code-for-ibmi.showLoginSettings"; it doesn't seem to be risky to open multiple editors (sorry...)

@worksofliam
Copy link
Contributor Author

@sebjulliand Updated!

@worksofliam worksofliam requested a review from sebjulliand May 14, 2024 12:42
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 and merged! 😎

@sebjulliand sebjulliand merged commit 7664f4b into master May 14, 2024
1 check passed
@sebjulliand sebjulliand deleted the cleanup/connection_manager_api branch May 14, 2024 13:10
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.

2 participants