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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to hide chosen columns in diagnostics panel #159

Merged
merged 6 commits into from
Jan 19, 2020

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Jan 18, 2020

Allows choosing which columns to display/hide in the diagnostic panel, using a context menu action.

References

Closes #149

Code changes

Greatly simplifies the DiagnosticListing.render() function body by moving the column-specific code to a newly created Column class (not exported). A new interface IListingContext facilitates this transition.

User-facing changes

Screenshot from 2020-01-18 11-59-47

Backwards-incompatible changes

None.

Chores

  • linted
  • tested
  • documented
  • changelog entry

@bollwyvl
Copy link
Collaborator

Cool!

Maybe on this PR:

This is definitely something that we would start to want to save in the settings. I guess the schema could be like:

{
  "title": "Language Server",
  "description": "Language Server Protocol settings.",
  "type": "object",
  "properties": {
    "diagnostics": {
      "type": "object",
      "properties": {
        "panel_columns": {
          "type": "array",
          "items": {"type": "string"},
          "default": ["virtual_document", "message", "code", "severity", "source", "cell", "line", "ch"]
        }
      }
    }
  }
}

Hiding a column would remove it from the list, showing would put it back. In the future, they could even be reorderable. It's helpful to have a lenient schema... what if we discover new columns that need adding later?

Probably on this PR:

A nit on the UI: multi-level menus only triggerable by right-click are a discoverability/accessibility nightmare... you've already built something, so keep it there, for sure! But perhaps as a lead-in to stuff down the road, have a small ... button right-most column that triggers the menu, and flatten everything into there. For now, each row would then be "wasted" space (but only 24x24).

Not on this PR:

Each row could eventually have a ... icon which had Hide messages like this... #128, Copy to clipboard #154, etc. that act at the row level.

The header-level button might then eventually reveal something like:

|---------------------------------|>[...]
|     Copy All To Clipboard       |
|     5 Messages Hidden...        |
|---------------------------------|
|     Show All Columns            |
| [x] Virtual Document            |
  //... more columns           //
| [x] Ch                          |
|---------------------------------|

@krassowski
Copy link
Member Author

krassowski commented Jan 18, 2020

  1. Settings and re-ordering: thought about it before, and I agree, but I wanted to keep the PRs small (so postponed for later).
  2. I was thinking about two ways for actions:
    • using toolbar icons as the area widget has a toolbar (as does the notebook, or the recent log console, see the log level filter screenshot here); I would prefer the proposed ... (for table-level actions such as columns selection) to go here (and maybe be more verbose than a hamburger?)
    • adding a dedicated column with buttons for [→] (jump to this diagnostic), [x] (hide this diagnostic" etc, but I think that this might be a waste of space and not the best UI. have not made up my mind yet, but definitely the next point (3):
  3. Sounds like a good idea. I was thinking about putting it all to the context menu, but why not both? Would you mind opening an issue for this?

Also, it might be beneficial to make a review of existing practices in other editors so that we know what the users expect.

Another quick thought: if we were to use three dots, what about the vertical arrangement (⋮) if these were to be placed in each row?

@krassowski
Copy link
Member Author

@bollwyvl I am not sure about the yarn lock file, if the way the new dependency ((still) @phosphor/menu) got addedd looks strange to you feel free to correct me and teach how to do it better

@@ -185,82 +277,58 @@ export class DiagnosticsListing extends VDomRenderer<DiagnosticsListing.Model> {
data: diagnostic_data,
key: virtual_document.uri + ',' + i,
document: virtual_document,
cell_nr: cell_nr
cell_nr: cell_nr,
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's an nr?

Copy link
Member Author

Choose a reason for hiding this comment

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

A leftover of my native language ;) number. Will expand the abbreviation.


return (
<table className={'lsp-diagnostics-listing'}>
<table className={DIAGNOSTICS_LISTING_CLASS}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1. I'm a big fan of squooshing all of them into a single file someplace, e.g. css.ts rather than strewing them all over the place.

@bollwyvl
Copy link
Collaborator

Settings and re-ordering:

Right. But trying to keep configurability in mind can save headaches down the line.

area widget has a toolbar

For the reasons mentioned elsewhere, more vertical height usually is a bummer. If it grows to a lot of things that need lots of explanation, perhaps. For example, if there was a filter box that let you add path:*.py to hide everything that wasn't currently python related, that would be worth my space... sometimes. So perhaps toolbar features could be enabled by context/hamburger.

I think that this might be a waste of space

If we're talking one button that reveals one flat menu, I'll take 24px of horizontal off vs 24px off vertical any day.

------------------------------------------------|
| document   | message   | line   | col   | ... |
|-----------------------------------------------|
| foo.py     | bad code  | 100    | 42    | ... |
|           -------------------------------/    | 
|           | Go to code                   |    |
| bar.py    | Copy to clipboard            |... |
|           | Filter messages like this... |    |
|           |------------------------------|    |
|

I think for complex sortable/filterable table, it's reasonable to have an escape hatch that lets you hide "table junk" until a user is ready for more complexity.

Would you mind opening an issue for this?

Incoming...

if we were to use three dots, what about the vertical arrangement (⋮) i

Ooh, look at that unicode. Good one. But yeah, that or something. I think people are pretty well trained at this point.

yarn lock

If you add new deps, and also since a release recently went out, it's fine. Since a release recently went out, and you're changing something, it would be fine to rebuild it.

@krassowski
Copy link
Member Author

If we're talking one button that reveals one flat menu

Agreed, my "waste of space comment" was about my previous idea of having multiple buttons, your proposal is totally fine!

I will still advocate the use of toolbar for coherence with other widgets in the JupyterLab, but I see a very important use case for #160 - if we decide to allow to dock the diagnostics listing in a sidebar panel then we might want not to have the toolbar and here it is a big advantage to have it as proposed.

PS. I am going to work on improving the completion now (a few hours this week and even more next week), also maybe "quick-fix" feature as those seem more urgent right now.

@krassowski krassowski merged commit 1c70b3b into master Jan 19, 2020
@krassowski krassowski deleted the diagnostics/hide_columns branch March 14, 2020 10:49
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.

Allow to hide columns in diagnostics panel
2 participants