Skip to content
This repository was archived by the owner on Aug 13, 2024. It is now read-only.

Fixes #340 - #341

Merged
sixinli merged 6 commits into
palantir:masterfrom
Rouche:master
Jan 16, 2017
Merged

Fixes #340#341
sixinli merged 6 commits into
palantir:masterfrom
Rouche:master

Conversation

@Rouche

@Rouche Rouche commented Jan 10, 2017

Copy link
Copy Markdown
Contributor

No description provided.

@palantirtech

palantirtech commented Jan 10, 2017

Copy link
Copy Markdown
Member

Thanks for your interest in palantir/eclipse-typescript, @Rouche! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@Rouche

Rouche commented Jan 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

CLA is signed

There are other bugs. Ill update the PR in a few

I think i fixed all. with this one. The only problem is i assumed the "tags" will be of type SymbolDisplayPart but i have no idea how to create a tag to appear in this list, and see whats in it. I am trying to find out.

All done!

sixinli
sixinli reviewed Jan 10, 2017
View reviewed changes
Comment thread com.palantir.typescript/src/com/palantir/typescript/services/language/QuickInfo.java
return this.tags;
}

public String getTagsText() {

@sixinli sixinli Jan 10, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

never use?

@Rouche Rouche Jan 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was unsure if you want to add the text in the Over. At least the method is there.

@sixinli sixinli Jan 11, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

by "Over" you mean "Overview"?

@Rouche Rouche Jan 11, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops... i mean the Hover lol. The popup that appear on Hovering. This method would have to be used to fill it with more info.

Sorry.

Comment thread com.palantir.typescript/src/com/palantir/typescript/services/language/JSDocTagInfo.java Outdated
@@ -0,0 +1,78 @@
/*
* Copyright 2013 Palantir Technologies, Inc.

@sixinli sixinli Jan 10, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

minor: 2017?

@Rouche Rouche Jan 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. But i did not touch other files.

@sixinli

sixinli commented Jan 10, 2017

Copy link
Copy Markdown
Collaborator

can you give an example of how to repro the error and how to test the fix?

@Rouche

Rouche commented Jan 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Pretty simple to test. Just use CTRL+SPACE after a dot. With this you will get the error.
There is no sign (popup) of the error, but the content assist window will never show. And you can see the error if you open Error Log view.

This code can create the error:

class JSDoc {

    /**
     * @private
     */
    private a: string;

    constructor() {
        this.a = "use ctrl+space here";
    }
}

@Rouche

Rouche commented Jan 11, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I found another bug:

When doing a show difference in Subversive SVN.
Will investigate.

Fixed too. Sweet, now the diff view show the syntax coloring!

java.lang.NullPointerException
	at com.palantir.typescript.text.reconciler.PresentationReconciler.getDamagedRegion(PresentationReconciler.java:132)
	at com.palantir.typescript.text.reconciler.PresentationReconciler.processEvent(PresentationReconciler.java:120)
	at com.palantir.typescript.text.reconciler.PresentationReconciler.access$0(PresentationReconciler.java:119)
	at com.palantir.typescript.text.reconciler.PresentationReconciler$MyTextListener.textChanged(PresentationReconciler.java:305)
	at org.eclipse.jface.text.TextViewer.updateTextListeners(TextViewer.java:2699)
	at org.eclipse.jface.text.TextViewer.fireRedrawChanged(TextViewer.java:4914)
	at org.eclipse.jface.text.TextViewer.disableRedrawing(TextViewer.java:4973)
	at org.eclipse.jface.text.TextViewer.setRedraw(TextViewer.java:4998)
	at org.eclipse.jface.text.TextViewer.setRedraw(TextViewer.java:4978)

sixinli
sixinli reviewed Jan 11, 2017
View reviewed changes

@sixinli sixinli left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

minor comments, lgtm

Comment thread com.palantir.typescript/src/com/palantir/typescript/text/reconciler/PresentationReconciler.java
private IRegion getDamagedRegion(TextEvent event) {
IDocument document = this.viewer.getDocument();
if(document == null) {
return null;

@sixinli sixinli Jan 11, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

minor preference on using optional instead

@Rouche Rouche Jan 11, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Gonna have to fill me in on this one. Been in TypeScript last year rusted my reflexes :P

@sixinli sixinli Jan 12, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

something like

if (document == null) {
    return Optional.absent();
}

it's preferred so that the caller would know that the value might not be present, so it can do

if (maybeDamagedRegion.isPresent()) {
    IRegion damagedRegion = this.getDamagedRegion(event).get();
    ...
}

it doesnt seem like anywhere else is using this pattern though so fine to keep it as it is for code consistency too

@Rouche Rouche Jan 12, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok i see.

Well, you know Optional is JDK 1.8, the plugin right now is still JDK 1.6, until i commit my update of Tycko to 0.26.0, wich is pending on this merge :) This one is a lot more important.

@sixinli sixinli Jan 12, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

guava's be fine (thats what everyone uses before JDK 1.8), but yea not really important :P

Comment thread com.palantir.typescript/src/com/palantir/typescript/services/language/QuickInfo.java
return this.tags;
}

public String getTagsText() {

@sixinli sixinli Jan 11, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

by "Over" you mean "Overview"?

@sixinli

sixinli commented Jan 16, 2017

Copy link
Copy Markdown
Collaborator

@Rouche is this good to go? or there's still more commits coming?

@Rouche

Rouche commented Jan 16, 2017

Copy link
Copy Markdown
Contributor Author

Its all good and working well. 1 week of testing and no issues. (Except the preference one, but for another adventure)

@sixinli

sixinli commented Jan 16, 2017

Copy link
Copy Markdown
Collaborator

thank you @Rouche ! will merge this for now, and figure out the tsconfig issue separately

@sixinli
sixinli merged commit 007579b into palantir:master Jan 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@sixinli sixinli sixinli approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Rouche @palantirtech @sixinli