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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Autocomplete to be properly registered as a Shortcut #2726

Merged
merged 12 commits into from
Sep 15, 2021

Conversation

thewheat
Copy link
Contributor

@thewheat thewheat commented Oct 15, 2020

Demo
autocomplete mov

Current issue
- If you modify the shortcut you need to reload Insomnia for it to register (still trying to find a way for this but open for guidance on this 馃槂) Fixed in 15e9e4f with some details here #2726 (comment)

const keymap = {
name: 'autocomplete-keymap',
"' '": functionMap.completeIfAfterTagOrVarOpen,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nijikokun
Copy link
Contributor

We might need a little piece of text or notice that a reload is required. cc @erickrico

@thewheat
Copy link
Contributor Author

@nijikokun I could definitely look into that route in this (or in another PR if this PR is something we'd want to merge first) 馃憤
I did some digging and wasn't able to find anything helpful to trigger a shortcut refresh in CodeMirror. So having a message may be an easier and quicker win for now

@thewheat
Copy link
Contributor Author

I've dug around a bit but haven't been able to find a good way to refresh the shortcuts on update but in testing further it looks like a full reload isn't necessary, we just need to select a new request and it works

image

If we were to display a message, how best should we do it? Tried mocking up this up in the settings section and it looks a bit odd if just add to the description
image

Perhaps the displaying of the message could be worked on in another PR? I'll set this PR as ready for review and if we want all the changes in this PR I'll be happy to make the change with some guidance on how best proceed 馃憤

@thewheat thewheat marked this pull request as ready for review October 24, 2020 09:26
@develohpanda
Copy link
Contributor

develohpanda commented Oct 24, 2020

Hi @thewheat! Sorry about the delay. There are a lot of PRs to get through and I haven't had a chance to work through them all yet. 馃槩

If you modify the shortcut you need to reload Insomnia for it to register (still trying to find a way for this but open for guidance on this 馃槂)

There is a uniquenessKey applied to the root component of the app to trigger a force refresh of the UI without the need to reload the app. You should be able to do something with altering this at an appropriate time. See the usages of forceRefreshCounter inside the file below.

const uniquenessKey = `${forceRefreshCounter}::${activeWorkspace._id}`;

By doing this, no messages will need to be shown to the user. I am anticipating that you might face an issue where the insomnia preferences modal automatically closes (because all the components will refresh). If that is the case, then I have another very similar solution to propose, but try re-use this for now.

@thewheat
Copy link
Contributor Author

No worries @develohpanda ! Thanks for the tip there, I'll try do some digging this weekend 馃憤

@CLAassistant
Copy link

CLAassistant commented Dec 6, 2020

CLA assistant check
All committers have signed the CLA.

@netlify
Copy link

netlify bot commented Feb 14, 2021

Deploy preview for insomnia-storybook ready!

Built with commit 85ae6af

https://deploy-preview-2726--insomnia-storybook.netlify.app

@thewheat
Copy link
Contributor Author

Took a long while for me to get back to this 馃槄 Digging around I saw that there was a method to refresh the wrapper

_forceRequestPaneRefresh(): void {
this.setState({ forceRefreshKey: Date.now() });
}

But using that didn't work. There was another method (shown below) that adds a 100ms delay before the refresh and that successfully got it working! 馃帀 So dediced to refactor that logic into its own method to be called in the existing location and in the new code when shortcuts are updated

async _handleForceUpdateRequest(r: Request, patch: Object): Promise<Request> {
const newRequest = await rUpdate(r, patch);
// Give it a second for the app to render first. If we don't wait, it will refresh
// on the old request and won't catch the newest one.
// TODO: Move this refresh key into redux store so we don't need timeout
window.setTimeout(this._forceRequestPaneRefresh, 100);
return newRequest;
}

If there was another way to approach this do let me know!

Demo
gif2

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Really great work, and no worries about the delay 馃檪

Looks good to me overall! There's only one blocker in my review, and that is to await the model update before triggering a refresh. The remaining items are just observations where I think it might be useful to add some code comments. 馃

@vercel vercel bot temporarily deployed to Preview September 8, 2021 11:40 Inactive
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

I pushed a few commits to replace the _forceRequestPaneRefreshAfterDelay approach. While testing I had noticed that the code-editor would briefly lose focus while typing immediately after changing a keyboard shortcut.

Instead I replaced the delay and force refresh approach by connecting the code-editor to settings.hotKeyRegistry in redux. I tested this a bit and it seems to be working well! Please have a look at the last few commits that add this!

2021-09-15 20 51 12

@gatzjames gatzjames merged commit 58ce5ec into Kong:develop Sep 15, 2021
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.

[BUG] Can't change keyboard binding for "Show Autocomplete"
6 participants