Skip to content

Added back support for Hyperclick #1265

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

Merged
merged 5 commits into from
Jun 25, 2017
Merged

Conversation

sverrejoh
Copy link
Contributor

@sverrejoh sverrejoh commented May 23, 2017

Hyperclick support dissapeared after a refactor.

Added the old code back, and updated it to work with the new API, as
used in go-to-definition. Included support for selecting between
multiple definitions.

Should fix #448 and #1253

  • All compiled assets are included (atom packages are git tags and hence the built files need to be a part of the source control)

Hyperclick support dissapeared after a refactor.

Added the old code back, and updated it to work with the new API, as
used in `go-to-definition`. Included support for selecting between
multiple definitions.
@mrmurphy
Copy link

mrmurphy commented May 31, 2017

Awesome! I was really missing hyperclick. One issue I just found, though. If you use hyperclick to navigate to a symbol definition, F10 doesn't return from the definition like it does when you use F12 to go do the definition. It shows this error instead:

atom beta_2017-05-31 13-56-33

AtomTS: Previous position not found

@sverrejoh
Copy link
Contributor Author

Right, I saw some state related to that in the jump-to-definition code. This should probably be unified somehow, and I can see what I can do :). Thanks for the feedback!

- Didn't cooperate with go-to-declaration, so return from declaration
  didn't work
- Actually used the position from the cursor, not the point of the click
@sverrejoh
Copy link
Contributor Author

@splodingsocks Thank you again for the feedback!

I fixed the issue with returning to definition with F10 and updated the pull request. Should be more solid now.

@mrmurphy
Copy link

Nice!!

@sverrejoh
Copy link
Contributor Author

@guncha Am I missing something in this pull request? I'm happy to clean it up if anything's missing.

@jonyeezs
Copy link

this should fix #1234 as well

Copy link
Contributor

@guncha guncha left a comment

Choose a reason for hiding this comment

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

Looks good, but needs a few more changes!


export function handleDefinitionResult(result: protocol.DefinitionResponse,
location: FileLocationQuery): void {
if (result.body!.length > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The optional body on the protocol.DefinitionResponse is there for a reason - Typescript might not return a definition in which case this will blow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I just copied it, but then it was in a promise, so I guess we can argue the exception was handled :).

Fixed it now.

wordRegExp: /([A-Za-z0-9_])+|['"`](\\.|[^'"`\\\\])*['"`]/g,
getSuggestionForWord(editor: AtomCore.IEditor, text: string, range: TextBuffer.IRange) {
const TS_GRAMMARS = ["source.ts", "source.tsx"];
if (TS_GRAMMARS.indexOf(editor.getGrammar().scopeName) === -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use isTypescriptGrammar helper from typescriptEditorPane.ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! I extracted it and put it in utils.

@guncha
Copy link
Contributor

guncha commented Jun 25, 2017

Solid PR! 👍

I could have sworn I wrote a comment about using the scopes from the Typescript grammar to narrow down the locations where Hyperclick is effective. Basically, when you return from the function, the range property is used to highlight with an underline the section of code to let the user know "hey, if you click here, I'll take you somewhere". Currently, it will highlight every word, including string literals, and not everything, when clicked, will result in a jump to definition.

That's something that would take this from 95% to 100%, but it can be a part of a separate PR. I'll release this so people can start using it.

@guncha guncha merged commit 99955af into TypeStrong:master Jun 25, 2017
@sverrejoh
Copy link
Contributor Author

Great suggestion 😊

I started looking into the scopes, and have something basic running. I'll try to come up with a list of what's not hyperclickable, and I can push it as a separate pull request. I think I need to add scopeDescriptorForBufferPosition to the TypeScript atom definition over at DefinitlyTyped for this as well.

From the top of my head:

  • String literals outside import statement
  • all other primitive types
  • keywords
  • within comments

Thanks a lot!

@guncha
Copy link
Contributor

guncha commented Jun 25, 2017

Sounds about right. You can always add it to the typings/atom_core.d.ts and then, if you want to be nice, open a PR against the DefinitelyTyped definitions.

@sompylasar sompylasar mentioned this pull request Jul 3, 2017
@TypeStrong TypeStrong locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement hyperclick package
4 participants