Diagnostics: handling of descriptions in messages #164

lloiser · 2018-02-08T21:51:53Z

This PR improves the handling of descriptions in diagnostic messages …

remove from table, only show in datatip/popup (description always rendered for messages in panel #40)
handle string and function types (LinterV2 compatibility checklist #45)
parse markdown using marked (LinterV2 compatibility checklist #45)

* remove from table, only show in datatip/popup * handle string and function types * parse markdown using marked

Added unnecessary line break between "Reference" and "<path>" if diagnostic message contains a reference

lloiser · 2018-02-08T21:58:38Z

In my opinion there are still some issues when I compare it to the linter-ui-default package:

font sizes (and font family) do not match my editor settings (more or less covered by Editor datatip does not use ui theme font size (but buffer data tip does) #23)
the width of the popup is max 500px. This causes longer texts that actually fit into the view to wrap. Some messages get really hard to read (especially flowtype output, as shown in the screens)

linter-ui-default

atom-ide-ui

Goom11 · 2018-02-08T22:13:26Z

This is awesome!! Thanks for doing this!

cc: @matthewwithanm, @hansonw
LGTM but y'all know this code better

Arcanemagus · 2018-02-08T22:14:37Z

remove from table, only show in datatip/popup

Ideally the fact that a message has a description should be showing in the table, it just should be collapsed by default. linter-ui-plus has a great interface for this, it just never implemented tooltips.

When #40 was filed descriptions always rendered their full contents with no way to collapse them. Currently there is a toggle that controls that behavior, the bug remaining there is that the first part of the description is still rendered when that option is disabled, but at least it is cut off.

This PR should not be marked as fixing #45, there are other parts of that that this doesn't cover. For example the url field is still ignored, even though it's one of the more useful bits of information from a message.

lloiser · 2018-02-09T08:18:52Z

Oh, I totally misunderstood #40 then. Sry...
I will see if I can get this expandable description into the table too.

lloiser · 2018-02-18T19:16:57Z

Whoa... it took me some time to get my head around rxjs. Hopefully I did not too bad 😆
The build failed because npm test died unexpectedly ... I unfortunately cannot retry the build.

codecov-io · 2018-02-21T07:41:10Z

Codecov Report

Merging #164 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #164      +/-   ##
=========================================
- Coverage    1.95%   1.94%   -0.01%     
=========================================
  Files         368     369       +1     
  Lines       14315   14363      +48     
=========================================
  Hits          280     280              
- Misses      14035   14083      +48

Impacted Files	Coverage Δ
...atom-ide-diagnostics/lib/services/LinterAdapter.js	`0% <ø> (ø)`	⬆️
...om-ide-diagnostics-ui/lib/ui/DiagnosticsMessage.js	`0% <ø> (ø)`	⬆️
.../pkg/atom-ide-diagnostics/lib/redux/createStore.js	`0% <ø> (ø)`	⬆️
modules/nuclide-commons/memoizeUntilChanged.js	`0% <ø> (ø)`	⬆️
...m-ide-ui/pkg/atom-ide-diagnostics-ui/lib/gutter.js	`0% <0%> (ø)`	⬆️
...ide-ui/pkg/atom-ide-diagnostics/lib/redux/Epics.js	`0% <0%> (ø)`	⬆️
...e-ui/pkg/atom-ide-diagnostics/lib/redux/Actions.js	`0% <0%> (ø)`	⬆️
...atom-ide-diagnostics-ui/lib/ui/DiagnosticsTable.js	`0% <0%> (ø)`	⬆️
...-ui/pkg/atom-ide-diagnostics/lib/redux/Reducers.js	`0% <0%> (ø)`	⬆️
...tom-ide-diagnostics-ui/lib/getDiagnosticDatatip.js	`0% <0%> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 094459b...3189a42. Read the comment docs.

lloiser · 2018-03-08T07:56:51Z

uhm.. I'm basically done with the PR, so it would nice if anybody can review it 😁

modules/atom-ide-ui/pkg/atom-ide-diagnostics-ui/lib/ui/DiagnosticsPopup.js

lloiser · 2018-03-29T07:26:54Z

Sry that I have to mention you. Is there any reason why this hasn't been reviewed yet? @matthewwithanm, @hansonw, @Goom11

matthewwithanm · 2018-03-29T17:02:08Z

really sorry about letting this slip, @lloiser! thanks so much for doing this. there's not a good reason we haven't reviewed it—just that it's a pretty big contribution and we've been busy.

i'll review today ❤️

matthewwithanm

thanks so much for putting the time into this, @lloiser ❤️

i left a few comments. lmk what you think 😊

matthewwithanm · 2018-03-30T01:06:13Z

modules/atom-ide-ui/pkg/atom-ide-diagnostics-ui/lib/main.js

          autoVisibility,
          onShowTracesChange: setShowTraces,
          onFilterByActiveTextEditorChange: setFilterByActiveTextEditor,
+          onSelectMessage: setSelectedMessage,


this is a little confusing since it makes it seem that setting the selected message is a global concern, when it isn't. it took me a couple read throughs to understand that we needed this because descriptions are a global concern. i think it would be cleared up some if you renamed setSelectedMessage() here to handleSelectMessage(). that way it's obvious that it's responding to an event and not updating state.

matthewwithanm · 2018-03-30T01:11:14Z

modules/atom-ide-ui/pkg/atom-ide-diagnostics-ui/lib/main.js

+            updater.fetchDescriptions([message], true);
+          }
+        };
+      });


we don't actually need to create a new function every time here. instead, we can just have a single function that closes over the state like this:

const setSelectedMessage = (message: DiagnosticMessage) => { const updater = this._model.state.diagnosticUpdater; if (updater == null) return; updater.fetchDescriptions([message], true); }

matthewwithanm · 2018-03-30T01:21:06Z

modules/atom-ide-ui/pkg/atom-ide-diagnostics-ui/lib/main.js

+        if (updater && showTraces) {
+          updater.fetchDescriptions(messages, false);
+        }
+      });


a couple things here:

this is a subscription that's never cleaned up.

this function is just for getting a stream of states—it really shouldn't have side effects like this.

fetching a description is now a potentially async (and uncancellable) operation. i don't think we can do this for every single message every time we get messages.

i'm not sure exactly what a good way to do this is though. 🤔@hansonw do you have any ideas?

I could easily add some cleanup code to this._subscriptions

It was rather easy to add it in here because it already contained all the nice streams to hook into :) but it could be moved somewhere else too.

it only fetches them if the check box is checked.

At least how it works in linter-ui-default and linter-ui-plus is that they are only fetched when requested by the user if they are a Promise, since there really is no way to know what is coming back from it.

The idea behind descriptions was that they could be a long form description for what is going on... to the point where the entire help web page could be loaded in there if the provider author really wanted. I don't know of any providers that take advantage of this, but there are ones that provide quite large text in there (linter-perlcritic being one of them).

sorry, i'm still not sure how to resolve this…if the checkbox is checked, this is going to do some potentially expensive async operation for every diagnostic, every time the diagnostics are updated. that last bit is pretty important because the diagnostics can be updated fairly frequently (e.g. if one is added).

it seems like maybe there's a disconnect about what "description" was intended for and what Nuclide's doing with it. if it truly is designed to be something that's only pulled, then maybe we should have another field for pushable, longer-form summaries? or we could expect summaries to be in a subject + newline + details format and the toggle would affect that…

cc @hansonw and @nmote for some context and advice on this

When I left Nuclide, diagnostics were just records with static information. There was no notion of fetching any additional information about diagnostics after the fact. So I don't think I have much to add here, sorry.

I somehow like and dislike the idea of having 2 "descriptions":

pro: it would make the diagnostics table as simple as it was before

pro: no async loading of many descriptions because they are not rendered in the table anymore

con: there would be 2 descriptions which is kind of hard to explain to a user why the diagnostics datatip description is different to the description in the table (but I'm no UI/UX expert 😬 )

I'm open to do as you wish, I just want the description to be part of the UI somewhere :)

Any updates on this? pending PRs are quite tedious...

I reverted the code responsible for showing the description in the table. It's only visible in the datatip. How does this sound to you?

matthewwithanm · 2018-03-30T01:24:56Z

modules/atom-ide-ui/pkg/atom-ide-diagnostics-ui/lib/ui/DiagnosticsMessageDescription.js

+) => {
+  const {description} = props;
+  if (description != null) {
+    const __html = marked(description);


can you use domPurify here?

also, since marked() is potentially expensive, we should probably make this PureComponent

matthewwithanm · 2018-03-30T01:28:35Z

modules/atom-ide-ui/pkg/atom-ide-diagnostics-ui/lib/ui/DiagnosticsPopup.js

+            goToLocation,
+            codeActionsForMessage,
+            descriptions,
+          ),


can you replace this bind call with an arrow function? i think it would be a lot easier to read if it were more explicit.

matthewwithanm · 2018-03-30T01:31:13Z

modules/atom-ide-ui/pkg/atom-ide-diagnostics/lib/redux/Epics.js

+      ),
+    )
+      .map(descriptions =>
+        Actions.setDescriptions(new Map(descriptions), keepDescriptions),


could you add a comment here explaining that (and why) we only do this once we get all of the descriptions?

The only reason was that having to load and dispatch a lot of descriptions updates the diagnostics table really often and (maybe) affects performance.

matthewwithanm · 2018-03-30T01:33:02Z

modules/atom-ide-ui/pkg/atom-ide-diagnostics/lib/redux/Epics.js

+      )
+      .catch(err => {
+        getLogger('atom-ide-diagnostics').error(
+          `Error fetching description for ${messages[0].filePath}`,


i think this catch should be on the individual description observable? otherwise the path won't necessarily be the one that errored, right?

matthewwithanm · 2018-03-30T02:03:12Z

modules/atom-ide-ui/pkg/atom-ide-diagnostics-ui/lib/DiagnosticsViewModel.js

+        selectMessage: (message: DiagnosticMessage) => {
+          this._selectMessage(message);
+          globalState.onSelectMessage(message);
+        },


hm, it's a bummer we're making this function every time we see an update, even if globalState.onSelectMessage hasn't changed. i guess we could do something like selectMessage: createSelectMessage(globalState.onSelectMessage), where

const createSelectMessage = memoizeUntilChanged( onSelectMessage => message => { this._model.setState({selectedMessage: message}); onSelectMessage(message); } );

(then we would just get rid of _selectMessage().) what do you think?

matthewwithanm · 2018-03-30T02:09:00Z

modules/atom-ide-ui/pkg/atom-ide-diagnostics-ui/lib/getDiagnosticDatatip.js

-      goToLocation: gotoLine,
-      codeActionsForMessage,
-    })),
+    Observable.zip(


i think you want combineLatest() here? otherwise you're depending on getting exactly one code action to every description.

* fix some minor rxjs issues * avoid unnecessary react props changes by memoizing props * use `domPurify` for description markdown

lloiser · 2018-04-02T11:16:04Z

I have noticed that since 792630d the description is not displayed anymore for selected messages in the diagnostics table. Are there any plans to add it back in? If not then I can remove the handleSelectMessage functions (and related code) I have added.

matthewwithanm · 2018-04-02T17:17:26Z

@lloiser nope! it wasn't a great user experience.

modules/atom-ide-ui/pkg/atom-ide-diagnostics-ui/lib/ui/DiagnosticsMessageText.js

It now is only visible in the datatip.

lloiser · 2018-07-08T18:40:20Z

@matthewwithanm as I already said above I reverted the changes in the table. The description is only visible in the datatip now.
The build failure is the same as on master currently. I guess this will be fixed with #273
Is there anything left for me to to?

matthewwithanm · 2018-07-08T18:42:55Z

Sorry, missed it! I'll check it out tomorrow ❤️

matthewwithanm · 2018-07-09T18:02:28Z

@lloiser we still show descriptions in the table when showTraces is true. what happens there with the async ones?

also, do we have a memory leak here? it seems like fetchDescriptions() is only ever called with keepDescriptions (should that be keepExisting?) === true. isn't that map just going to grow indefinitely?

lloiser · 2018-07-09T18:22:32Z

As I said, the diagnostics table does the same as before: takes the message and splits on new lines and uses that as description. Only the datatip contains the description.
Yes you are right there is memory leak... I'm on it!

Whenever the list of messages changes, check if the messages of the descriptions are still valid.

lloiser · 2018-07-09T20:04:20Z

@matthewwithanm now with memory leak prevention 😄

matthewwithanm · 2018-07-10T00:38:48Z

modules/atom-ide-ui/pkg/atom-ide-diagnostics/lib/redux/Epics.js

+          });
+        });
+      });
+      if (descriptions.size === newDescriptions.size) {


isn't it possible for them to be the same size but still have changed?

I can't think of any case where this can happen...

matthewwithanm

thanks for sticking with us! ❤️

one quick q

matthewwithanm · 2018-07-11T19:33:05Z

okay, so i'm testing this out, trying to merge and not seeing the descriptions 🤔

probably i'm just doing something dumb, but if you do this and then hover the gutter, they should be populated, right?

atom.packages.serviceHub.consume('linter-indie', '2.0.0', x => { window.linter = x({name: 'Test'}); });
// then…
linter.setAllMessages([{
  severity: 'info',
  location: {
    file: '/path/to/file.js',
    position: [[5, 0]],
  },
  excerpt: 'This is an error message on a file',
  description: () => Promise.resolve('blah')
}]);

lloiser · 2018-07-12T05:59:53Z

You are right, in the gutter it does not work but when you hover the code in the editor blah is shown.
Ah... I see it uses a completely different code path. Back to coding then :)

lloiser · 2018-07-17T12:02:57Z

@matthewwithanm ping ...

(sry for being annoying 😬)

matthewwithanm · 2018-07-23T23:35:38Z

@lloiser no need to apologize! sorry this has taken so long. this is an important part of our workflows at facebook and we just need to be extra careful (:

Summary: Imported from facebookarchive/atom-ide-ui#164, this supports linter messages with async descriptions. Reviewed By: hansonw Differential Revision: D8825841 fbshipit-source-id: 622acf765285a21f39d643f11682e589945425e9

Summary: Imported from #164, this supports linter messages with async descriptions. Reviewed By: hansonw Differential Revision: D8825841 fbshipit-source-id: 622acf765285a21f39d643f11682e589945425e9

lloiser added 2 commits February 8, 2018 22:39

Improve handling of description in diagnostic messages

bc48d71

* remove from table, only show in datatip/popup * handle string and function types * parse markdown using marked

Avoid adding a br tag after the last message text line

ee0d5e7

Added unnecessary line break between "Reference" and "<path>" if diagnostic message contains a reference

facebook-github-bot added the CLA Signed label Feb 8, 2018

Arcanemagus mentioned this pull request Feb 8, 2018

Set max-width for linter tooltip for readability. steelbrain/linter-ui-default#467

Merged

lloiser changed the title ~~Diagnostics: handling of descriptions in messages~~ WIP: Diagnostics: handling of descriptions in messages Feb 9, 2018

lloiser added 2 commits February 9, 2018 22:34

Fix linter adapter spec to have a separate description field

e97084b

add description in the diagnostics table

fb189b9

Merge remote-tracking branch 'origin/master' into ll-description

8eb2053

lloiser changed the title ~~WIP: Diagnostics: handling of descriptions in messages~~ Diagnostics: handling of descriptions in messages Feb 22, 2018

Merge remote-tracking branch 'origin/master' into ll-description

8e460ac

modules/atom-ide-ui/pkg/atom-ide-diagnostics-ui/lib/ui/DiagnosticsPopup.js

matthewwithanm suggested changes Mar 30, 2018

View reviewed changes

lloiser added 2 commits April 1, 2018 17:39

Merge remote-tracking branch 'origin/master' into ll-description

0d1752f

review suggestions

0434908

* fix some minor rxjs issues * avoid unnecessary react props changes by memoizing props * use `domPurify` for description markdown

lloiser added 2 commits April 3, 2018 21:59

remove selection handler (used to load the description)

ebe7af9

Merge remote-tracking branch 'origin/master' into ll-description

076ff8b

modules/atom-ide-ui/pkg/atom-ide-diagnostics-ui/lib/ui/DiagnosticsMessageText.js

lloiser mentioned this pull request May 2, 2018

Datatip displays plain HTML steelbrain/flow-ide#139

Closed

lloiser added 3 commits June 4, 2018 08:44

Merge remote-tracking branch 'origin/master' into ll-description

c739ef7

include descriptions in the DiagnosticView spec

09653df

pretty(ier)

31f3799

lloiser added 2 commits June 27, 2018 22:51

Merge remote-tracking branch 'origin/master' into ll-description

c795b4e

Revert code to show the description in the table.

cc724a2

It now is only visible in the datatip.

prevent memory leaks of descriptions

e06067d

Whenever the list of messages changes, check if the messages of the descriptions are still valid.

matthewwithanm reviewed Jul 10, 2018

View reviewed changes

show the diagnostic description also in the gutter popup

a7db89e

Merge remote-tracking branch 'origin/master' into ll-description

3189a42

matthewwithanm merged commit 9ab1156 into facebookarchive:master Jul 23, 2018

lloiser deleted the ll-description branch August 23, 2018 20:41

Diagnostics: handling of descriptions in messages #164

Diagnostics: handling of descriptions in messages #164

Uh oh!

Conversation

lloiser commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lloiser commented Feb 8, 2018

Uh oh!

Goom11 commented Feb 8, 2018

Uh oh!

Arcanemagus commented Feb 8, 2018

Uh oh!

lloiser commented Feb 9, 2018

Uh oh!

lloiser commented Feb 18, 2018

Uh oh!

codecov-io commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lloiser commented Mar 8, 2018

Uh oh!

lloiser commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthewwithanm commented Mar 29, 2018

Uh oh!

matthewwithanm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lloiser May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lloiser commented Apr 2, 2018

Uh oh!

matthewwithanm commented Apr 2, 2018

Uh oh!

lloiser commented Jul 8, 2018

Uh oh!

matthewwithanm commented Jul 8, 2018

Uh oh!

matthewwithanm commented Jul 9, 2018

Uh oh!

lloiser commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lloiser commented Feb 8, 2018 •

edited

Loading

codecov-io commented Feb 21, 2018 •

edited

Loading

lloiser commented Mar 29, 2018 •

edited

Loading

lloiser May 11, 2018 •

edited

Loading

lloiser commented Jul 9, 2018 •

edited

Loading