Handle markdown in LSP doc popups #10645

tsmaeder · 2018-08-03T09:00:28Z

What does this PR do?

Adds handling of markdown in the widgets that show additional doc (i.e. javadoc) for completions and signature help in LSP editors.

What issues does this PR fix or reference?

#10499

Signed-off-by: Thomas Mäder <tmader@redhat.com>

evidolob · 2018-08-03T09:11:54Z

...e/che/plugin/languageserver/ide/editor/codeassist/CompletionItemBasedCompletionProposal.java

-    } else {
-      callback.onSuccess(createAdditionalInfoWidget());
-    }
+    MarkedOverlay.getMarkedPromise()


Maybe you should provide some sort of abstraction instead using MarkedOverlay directly.
It's OK to use MarkedOverlay in Orion module, but in this module we shouldn't have direct dependency on Orion .

You're right, architecturally, but I would propose to merge it like that and to open an issue for it:

I'm on PTO in around 2 hours and I'd like to get this merged

Investing in architectural integrity of the gwt ide seems low prio.

evidolob · 2018-08-03T09:13:12Z

...re-orion-editor/src/main/java/org/eclipse/che/ide/editor/orion/client/jso/MarkedOverlay.java

+ */
+package org.eclipse.che.ide.editor.orion.client.jso;
+
+/**


Please, place javadoc after imports.

Strange that the formatter does not reorder that.

tolusha · 2018-08-03T09:05:02Z

...re-orion-editor/src/main/java/org/eclipse/che/ide/editor/orion/client/jso/MarkedOverlay.java

+
+  protected MarkedOverlay() {}
+
+  public static Promise<MarkedOverlay> getMarkedPromise() {


'getReady()` sounds better

Other classes use this pattern, see org.eclipse.che.ide.editor.orion.client.EditorInitializePromiseHolder.getInitializerPromise()

tolusha · 2018-08-03T09:05:32Z

...re-orion-editor/src/main/java/org/eclipse/che/ide/editor/orion/client/jso/MarkedOverlay.java

+ */
+package org.eclipse.che.ide.editor.orion.client.jso;
+
+/**


Should be before class declaration

dmytro-ndp · 2018-08-03T09:42:25Z

selenium/che-selenium-test/src/main/java/org/eclipse/che/selenium/pageobject/CodenvyEditor.java

@@ -566,9 +566,7 @@ public void runActionForTabFromContextMenu(TabActionLocator tabAction) {
   * @param text text which should be typed
   */
  public void typeTextIntoEditor(String text) {
-    loader.waitOnClosed();


loader.waitOnClosed(); is needed here to be sure the loader is not displayed in front of editor, or it is gone after the text has been pasted.

loader.waitOnClosed() does at least a one second wait. On this one, I am not sure, but I'd like to try removing it. We can reenable it if the test start failing.
Generally, I think this is the wrong approach. We should be testing for conditions that we need, for example we want the editor to have keyboard focus.

dmytro-ndp · 2018-08-03T09:49:34Z

selenium/che-selenium-test/src/main/java/org/eclipse/che/selenium/pageobject/CodenvyEditor.java

@@ -897,7 +894,7 @@ public void selectItemIntoAutocompleteAndPerformDoubleClick(String item) {
   */
  public void selectAutocompleteProposal(String item) {
    seleniumWebDriverHelper.waitAndClick(
-        By.xpath(format(AUTOCOMPLETE_CONTAINER + "/li/span[text()='%s']", item)));


Could you, please, explain what text() was replaced by . in xpath "/li/span[text()='%s']" for?

. recursively takes the text in all subelements ("foo <span>bar</span>" is "foo bar", whereas in "text()" it woudl be "foo ".

I see, thank you for explanation.

dmytro-ndp · 2018-08-03T09:53:23Z

...nium-core/src/main/java/org/eclipse/che/selenium/core/webdriver/SeleniumWebDriverHelper.java

    webDriverWaitFactory
        .get(timeout)
+        .withMessage(
+            () -> {
+              return "expected \n'" + expectedText + "'\nbut was \n'" + actual[0] + "'\n";


Great idea!

I introduced this in order to see why the teste failed. It makes fixing much more convenient.

dmytro-ndp · 2018-08-03T09:57:45Z

...ium/che-selenium-test/src/main/java/org/eclipse/che/selenium/pageobject/ProjectExplorer.java

@@ -290,8 +290,6 @@ public void waitItem(String path) {
   * @param timeout waiting timeout in seconds
   */
  public void waitItem(String path, int timeout) {
-    loader.waitOnClosed();


We need to ensure that there is no loader in front of IDE to make the tests more stable.

No, that makes no sense! In Pseudocode, this would read:

wait for condition x

wait for condition y.

if we never do anything with "x", we can just remove the first wait. If the wait is too short, then, we should increase the timeout.

This method is being called from other tests which rely on loader.waitOnClosed(). If you are removing this wait from the method, you should add loader.waitOnClosed() to the callers.

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added 2 commits August 3, 2018 10:58

Use Orion's "marked" to support markdown in doc popups

831d7e5

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Remove redundant waits

6586043

Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder requested review from svor and tolusha August 3, 2018 09:00

tsmaeder requested review from dkuleshov and vparfonov as code owners August 3, 2018 09:00

tsmaeder requested a review from evidolob August 3, 2018 09:02

evidolob reviewed Aug 3, 2018

View reviewed changes

tolusha approved these changes Aug 3, 2018

View reviewed changes

dmytro-ndp reviewed Aug 3, 2018

View reviewed changes

tsmaeder merged commit 66f7bfd into eclipse-che:5730_java_ls_poc Aug 3, 2018

tsmaeder mentioned this pull request Aug 3, 2018

Fixup for 10499 handle markdown #10655

Merged

tolusha pushed a commit that referenced this pull request Aug 7, 2018

Handle markdown in LSP doc popups (#10645)

a9b2639

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

benoitf added the target/branch Indicates that a PR will be merged into a branch other than master. label Aug 9, 2018

tsmaeder added a commit that referenced this pull request Aug 22, 2018

Handle markdown in LSP doc popups (#10645)

e2d548c

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Aug 29, 2018

Handle markdown in LSP doc popups (#10645)

a6e5f72

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tolusha pushed a commit that referenced this pull request Sep 5, 2018

Handle markdown in LSP doc popups (#10645)

d579e05

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Sep 13, 2018

Handle markdown in LSP doc popups (#10645)

607cb68

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Sep 20, 2018

Handle markdown in LSP doc popups (#10645)

229c0a4

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Sep 26, 2018

Handle markdown in LSP doc popups (#10645)

fd63dd4

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Oct 1, 2018

Handle markdown in LSP doc popups (#10645)

1a9bf2c

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Oct 5, 2018

Handle markdown in LSP doc popups (#10645)

9b37dd4

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Oct 12, 2018

Handle markdown in LSP doc popups (#10645)

eb19370

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Oct 16, 2018

Handle markdown in LSP doc popups (#10645)

e165069

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Oct 17, 2018

Handle markdown in LSP doc popups (#10645)

b54762d

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Oct 17, 2018

Handle markdown in LSP doc popups (#10645)

8aab865

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Oct 17, 2018

Handle markdown in LSP doc popups (#10645)

e724840

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Oct 17, 2018

Handle markdown in LSP doc popups (#10645)

5dbf5cc

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added a commit that referenced this pull request Oct 17, 2018

Handle markdown in LSP doc popups (#10645)

58aa76d

* Use Orion's "marked" to support markdown in doc popups Signed-off-by: Thomas Mäder <tmader@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle markdown in LSP doc popups #10645

Handle markdown in LSP doc popups #10645

tsmaeder commented Aug 3, 2018

evidolob Aug 3, 2018

tsmaeder Aug 3, 2018

evidolob Aug 3, 2018

tsmaeder Aug 3, 2018

tolusha Aug 3, 2018

tsmaeder Aug 3, 2018

tolusha Aug 3, 2018

dmytro-ndp Aug 3, 2018

tsmaeder Aug 3, 2018

dmytro-ndp Aug 3, 2018

tsmaeder Aug 3, 2018 •

edited

Loading

dmytro-ndp Aug 6, 2018

dmytro-ndp Aug 3, 2018

tsmaeder Aug 3, 2018

dmytro-ndp Aug 3, 2018 •

edited

Loading

tsmaeder Aug 3, 2018 •

edited

Loading

dmytro-ndp Aug 6, 2018


		protected MarkedOverlay() {}

		public static Promise<MarkedOverlay> getMarkedPromise() {

Handle markdown in LSP doc popups #10645

Handle markdown in LSP doc popups #10645

Conversation

tsmaeder commented Aug 3, 2018

What does this PR do?

What issues does this PR fix or reference?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmytro-ndp Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

tsmaeder Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder Aug 3, 2018 •

edited

Loading

dmytro-ndp Aug 3, 2018 •

edited

Loading

tsmaeder Aug 3, 2018 •

edited

Loading