Add Quick Connect #1150

chrjorgensen · 2023-03-21T18:58:16Z

Changes

This PR will add a Quick Connect setting in the connection configuration to speed up connecting to the system:

When selected, the following server settings are taken from the previous connection to the system:

bad data areas
all remote features
ASP info
CCSID
encoding values for #@$

These values are stored in connection storage on each connection to the system. So if no previous connection has been done, even though the Quick Connect is checked, all settings will be retrieved from the server and stored in the connection configuration to be used on the next connection.

This setting is active by default. The user can disable it permanently in the configuration, or the user can reload all settings from server into cache by right-clicking on the connection and select Connect and Reload Server Settings.

If the remote feature keys do not match the keys in the cache, the remote features will be checked on the server disregarding this setting.

For more information, see issue #1122.

Checklist

have tested my change
eslint is not complaining
for feature PRs: PR only includes one feature enhancement.

worksofliam · 2023-03-23T01:42:58Z

@chrjorgensen This is awesome. I personally love that this is an opt in thing. Please fix up the conflicts and then I will retest again.

chrjorgensen · 2023-03-23T16:51:01Z

@worksofliam Conflicts fixed, please retest...

sebjulliand

That's an awesome change and should indeed speed up connection times. Great job!

Maybe you can make a change to enhance the UX a bit more. Having to uncheck the property in the settings, connect and check again the property seems a bit cumbersome. How about having a Connect and reload settings command that could force the reloading of the settings even if quick connect is on? This could be a right click action in the connection browser.

chrjorgensen · 2023-03-25T14:35:37Z

@sebjulliand I now added a connection right-click option to connect and reload server settings:

A new command ConnectToAndReload has been created, but is not available in the command palette, only on the right-click menu.

I had to add one parameter to the connect functions to specify that server settings should be reloaded, despite quick connect was set for the connection. Could I have done it better in another way?

src/api/Storage.ts

sebjulliand

It's all good to me; just a suggestion to consider.

sebjulliand · 2023-03-25T15:36:56Z

I had to add one parameter to the connect functions to specify that server settings should be reloaded, despite quick connect was set for the connection. Could I have done it better in another way?

No, that's perfect the way you did it @chrjorgensen !

sebjulliand · 2023-03-25T15:47:42Z

@chrjorgensen, @worksofliam : shouldn't Quick connect actually be turned on by default when creating a connection? I'd tend to think so. What do you guys think?

worksofliam · 2023-03-25T15:48:48Z

@sebjulliand I am guessing this quick connect would only work after the initial connection so we have some system information.

I haven't tested this PR yet

sebjulliand · 2023-03-25T15:56:08Z

@sebjulliand I am guessing this quick connect would only work after the initial connection so we have some system information.

If it's on but nothing's been cached yet, it will perform a normal connection to gather and cache the values. That's why I think it doesn't hurt to have it turned on by default.

chrjorgensen · 2023-03-25T16:38:57Z

Yeah, Quick connect will not break if activated on a connection never used before. For each setting it will check if there's a value in the cache from a previous connection and only then use that value. Otherwise it will go to the server as usual.
So having this on by default will not break anything.

I'm undecided whether this setting is best suited as a connection setting or a global setting. Since it won't break anything and servers very, very rarely change settings, this could just as well be a global setting. What do you guys think?

sebjulliand · 2023-03-25T17:53:32Z

When you put it this way, I think it should even not be an option at all, but just the built-in behavior. I can't think of a scenario where you'd need to reload the cached settings every time. Since we have the command to reload them on demand, there may be no point to have it as an option at all.

worksofliam · 2023-03-25T18:50:56Z

When you put it this way, I think it should even not be an option at all, but just the built-in behavior.

I somewhat agree with it. In fact, I was thinking an option like 'always do a full connect' (which means never use our cached settings) in the VS Code settings would work here. If it's false, it will: use the cached settings if they exist, otherwise do full connect and store them. If it's true.. always do a full connect

chrjorgensen · 2023-04-03T23:30:35Z

@sebjulliand This PR was so far behind master that I had to rebase the changes onto master branch and force push.
I also added a check for remote features in the cache, since new features have been and will be added in the future, and then the cache is not valid.

sebjulliand

Just a small suggestion and conflicts to resolve; I can't wait to merge this PR, it's so convenient!

src/api/IBMi.ts

sebjulliand

Everything is fine, save for one detail; it's not directly related to your PR, but it's been highlighted by it 😉

In src/webviews/settings/index.ts, the target configuration doesn't get loaded and saved in all cases using the ConnectionConfiguration.load and ConnectionConfiguration.update method. Since ConnectionConfiguration.load isn't used to load the configuration, some default values are not applied for undefined settings, like Quick Connect. Consequently, when you edit the connection settings for a connection for which Quick Connect is undefined, the checkbox is unchecked. But when the settings will be loaded during the connection, it will actually be true. So the settings page is not the exact reflect of the actual settings in this case 😉

Using ConnectionConfiguration's load and update will also simplify the connection settings page code a little bit and remove some redundant code.

chrjorgensen · 2023-04-09T21:52:03Z

@sebjulliand I see the problem and is on it...

chrjorgensen · 2023-04-11T14:27:08Z

@sebjulliand The initialization problem in settings view is now fixed - please test thoroughly, since this is not a trivial code rewrite.

chrjorgensen · 2023-04-11T14:28:27Z

@worksofliam It would be nice to have your eyes on this as well.

worksofliam · 2023-04-11T14:29:18Z

@chrjorgensen Expect a test tonight (my tonight!) 👍

chrjorgensen · 2023-04-11T14:42:20Z

@worksofliam During my own tests I noticed that the fields enableSQL, showDescInLibList and tempDir asks for a reload of the workspace. But the connection is not reloaded (anymore?), should it not reload? Instead VS Code is being reloaded.

I also noticed that showDescInLibList actually does not require a reload, just a refresh of the library list view (because I coded the library description feature! 😆). I will make a PR for this later, when this PR has been merged. But are there any more fields, that does not require reload?

worksofliam · 2023-04-11T14:48:07Z

@chrjorgensen I had a quick review and I think the whole idea of restarting can be removed now - you're right. I would need to confirm this for enableSQL as we use that in a lot of places!

src/webviews/settings/index.ts

sebjulliand

The code is good and it works fine.
There is just one thing that needs to be fixed in my opinion, and we'll be good to merge!

Co-authored-by: Sébastien Julliand <sebjulliand@gmail.com>

chrjorgensen · 2023-04-11T16:27:57Z

@sebjulliand Suggestion has been accepted! 😉

sebjulliand

I'm all for it!
@worksofliam I'll let you make your round of tests and merge if you're OK with it.

worksofliam · 2023-04-11T17:41:45Z

src/api/Configuration.ts

@@ -119,7 +120,8 @@ export namespace ConnectionConfiguration {
      debugIsSecure: (parameters.debugIsSecure === true),
      debugUpdateProductionFiles: (parameters.debugUpdateProductionFiles === true),
      debugEnableDebugTracing: (parameters.debugEnableDebugTracing === true),
-      readOnlyMode: (parameters.readOnlyMode === true)
+      readOnlyMode: (parameters.readOnlyMode === true),
+      quickConnect: (parameters.quickConnect === true || parameters.quickConnect === undefined)


I tested this and it shows that Quick Connect will be on by default.

@worksofliam Yeah, Quick Connect was changed to be the default behavior with the setting available for the user to opt-out - per our discussion two weeks ago. Hope this is okay?

worksofliam

Code looks good, but the 'Connect and Reload' right click option doesn't work as expected. Seems like the UI isn't updating after the connection is made?

Video uploading here: https://youtu.be/wntqwrjuFdM

chrjorgensen · 2023-04-11T18:48:53Z

Code looks good, but the 'Connect and Reload' right click option doesn't work as expected. Seems like the UI isn't updating after the connection is made?

Video uploading here: https://youtu.be/wntqwrjuFdM

@worksofliam I see that - looking into it...

chrjorgensen · 2023-04-11T19:32:04Z

@worksofliam I'm glad I asked you to have a look and do some testing! 😃 There were a problem with two optional parameters to connect, one of which were created in master branch after the other were created in my PR branch - and since they were both boolean and optional, no warning were issued during development!

It should have been fixed now, please retest...

sebjulliand · 2023-04-11T20:32:03Z

@worksofliam I'm glad I asked you to have a look and do some testing! 😃 There were a problem with two optional parameters to connect, one of which were created in master branch after the other were created in my PR branch - and since they were both boolean and optional, no warning were issued during development!

It should have been fixed now, please retest...

Oh that's right. That was the reconnect boolean I added a few weeks ago, after your PR was created, to enhance the re-connection process after the connection gets dropped.

It's good you pinpointed it! Now it makes me think that when we start having a few options like that, especially a succession of optional ones, we should gather them in an object to avoid this kind of issues and prevent "merge messes" 😅

//For example this...
async connect(connectionObject: ConnectionData, reconnecting?: boolean, reloadServerSettings: boolean = false) {
...
}

//...could become this
async connect(connectionObject: ConnectionData, options?:{reconnecting?: boolean, reloadServerSettings?: boolean}) {
...
}

Not for today but definitely something way may keep in mind 😉

sebjulliand

Connect to reload cache works fine now.
Disconnecting and reconnecting manually with quick connect on works as expected.

worksofliam

Yup, looks like it is working as expected now. I really like this feature - thanks for your hard work @chrjorgensen!

I will give you the pleasure of merging.

chrjorgensen · 2023-04-11T21:53:38Z

I will give you the pleasure of merging.

Thanks @worksofliam and @sebjulliand

chrjorgensen added the enhancement New feature or request label Mar 21, 2023

chrjorgensen requested review from sebjulliand and worksofliam March 21, 2023 18:58

worksofliam linked an issue Mar 23, 2023 that may be closed by this pull request

Replace SQL usage of fetching system values with QSH sysval #1122

Closed

worksofliam assigned chrjorgensen Mar 23, 2023

chrjorgensen assigned worksofliam and unassigned chrjorgensen Mar 23, 2023

sebjulliand requested changes Mar 23, 2023

View reviewed changes

chrjorgensen requested a review from sebjulliand March 25, 2023 14:35

sebjulliand reviewed Mar 25, 2023

View reviewed changes

src/api/Storage.ts Outdated Show resolved Hide resolved

src/api/Storage.ts Outdated Show resolved Hide resolved

sebjulliand reviewed Mar 25, 2023

View reviewed changes

src/api/Storage.ts Outdated Show resolved Hide resolved

sebjulliand reviewed Mar 25, 2023

View reviewed changes

src/api/Storage.ts Outdated Show resolved Hide resolved

sebjulliand previously approved these changes Mar 25, 2023

View reviewed changes

chrjorgensen dismissed sebjulliand’s stale review via a7f7a63 March 25, 2023 16:33

worksofliam removed their assignment Mar 25, 2023

chrjorgensen force-pushed the feature/quick-connect branch from a7f7a63 to b8fd440 Compare April 3, 2023 23:28

chrjorgensen requested a review from sebjulliand April 3, 2023 23:30

chrjorgensen assigned sebjulliand Apr 3, 2023

sebjulliand requested changes Apr 4, 2023

View reviewed changes

src/api/IBMi.ts Outdated Show resolved Hide resolved

sebjulliand requested changes Apr 9, 2023

View reviewed changes

Initialize new config parameters in settings view

f8469c7

chrjorgensen requested a review from sebjulliand April 11, 2023 14:27

sebjulliand reviewed Apr 11, 2023

View reviewed changes

src/webviews/settings/index.ts Outdated Show resolved Hide resolved

sebjulliand requested changes Apr 11, 2023

View reviewed changes

Fix update of current config

fb6adfb

Co-authored-by: Sébastien Julliand <sebjulliand@gmail.com>

chrjorgensen requested a review from sebjulliand April 11, 2023 16:27

sebjulliand previously approved these changes Apr 11, 2023

View reviewed changes

worksofliam reviewed Apr 11, 2023

View reviewed changes

worksofliam approved these changes Apr 11, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

Fix optional parameter in call to 'connect'

dc67986

chrjorgensen dismissed sebjulliand’s stale review via dc67986 April 11, 2023 19:28

chrjorgensen requested review from worksofliam and sebjulliand April 11, 2023 19:32

sebjulliand approved these changes Apr 11, 2023

View reviewed changes

worksofliam approved these changes Apr 11, 2023

View reviewed changes

chrjorgensen merged commit 8f25b60 into codefori:master Apr 11, 2023

chrjorgensen deleted the feature/quick-connect branch April 13, 2023 08:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Quick Connect #1150

Add Quick Connect #1150

chrjorgensen commented Mar 21, 2023 •

edited

Loading

worksofliam commented Mar 23, 2023

chrjorgensen commented Mar 23, 2023

sebjulliand left a comment

chrjorgensen commented Mar 25, 2023

sebjulliand left a comment

sebjulliand commented Mar 25, 2023

sebjulliand commented Mar 25, 2023

worksofliam commented Mar 25, 2023

sebjulliand commented Mar 25, 2023

chrjorgensen commented Mar 25, 2023

sebjulliand commented Mar 25, 2023

worksofliam commented Mar 25, 2023

chrjorgensen commented Apr 3, 2023 •

edited

Loading

sebjulliand left a comment •

edited

Loading

sebjulliand left a comment

chrjorgensen commented Apr 9, 2023

chrjorgensen commented Apr 11, 2023

chrjorgensen commented Apr 11, 2023

worksofliam commented Apr 11, 2023

chrjorgensen commented Apr 11, 2023

worksofliam commented Apr 11, 2023

sebjulliand left a comment

chrjorgensen commented Apr 11, 2023

sebjulliand left a comment •

edited

Loading

worksofliam Apr 11, 2023

chrjorgensen Apr 11, 2023

worksofliam left a comment

This comment was marked as duplicate.

chrjorgensen commented Apr 11, 2023

chrjorgensen commented Apr 11, 2023

sebjulliand commented Apr 11, 2023

sebjulliand left a comment

worksofliam left a comment

chrjorgensen commented Apr 11, 2023

Add Quick Connect #1150

Add Quick Connect #1150

Conversation

chrjorgensen commented Mar 21, 2023 • edited Loading

Changes

Checklist

worksofliam commented Mar 23, 2023

chrjorgensen commented Mar 23, 2023

sebjulliand left a comment

Choose a reason for hiding this comment

chrjorgensen commented Mar 25, 2023

sebjulliand left a comment

Choose a reason for hiding this comment

sebjulliand commented Mar 25, 2023

sebjulliand commented Mar 25, 2023

worksofliam commented Mar 25, 2023

sebjulliand commented Mar 25, 2023

chrjorgensen commented Mar 25, 2023

sebjulliand commented Mar 25, 2023

worksofliam commented Mar 25, 2023

chrjorgensen commented Apr 3, 2023 • edited Loading

sebjulliand left a comment • edited Loading

Choose a reason for hiding this comment

sebjulliand left a comment

Choose a reason for hiding this comment

chrjorgensen commented Apr 9, 2023

chrjorgensen commented Apr 11, 2023

chrjorgensen commented Apr 11, 2023

worksofliam commented Apr 11, 2023

chrjorgensen commented Apr 11, 2023

worksofliam commented Apr 11, 2023

sebjulliand left a comment

Choose a reason for hiding this comment

chrjorgensen commented Apr 11, 2023

sebjulliand left a comment • edited Loading

Choose a reason for hiding this comment

worksofliam Apr 11, 2023

Choose a reason for hiding this comment

chrjorgensen Apr 11, 2023

Choose a reason for hiding this comment

worksofliam left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

chrjorgensen commented Apr 11, 2023

chrjorgensen commented Apr 11, 2023

sebjulliand commented Apr 11, 2023

sebjulliand left a comment

Choose a reason for hiding this comment

worksofliam left a comment

Choose a reason for hiding this comment

chrjorgensen commented Apr 11, 2023

chrjorgensen commented Mar 21, 2023 •

edited

Loading

chrjorgensen commented Apr 3, 2023 •

edited

Loading

sebjulliand left a comment •

edited

Loading

sebjulliand left a comment •

edited

Loading