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

hydrogen:clear-results removes results from all panes #1496

Closed
1 of 3 tasks
kylebarron opened this issue Dec 18, 2018 · 6 comments · Fixed by #1547
Closed
1 of 3 tasks

hydrogen:clear-results removes results from all panes #1496

kylebarron opened this issue Dec 18, 2018 · 6 comments · Fixed by #1547

Comments

@kylebarron
Copy link
Contributor

kylebarron commented Dec 18, 2018

Edit: I originally discovered this behavior from starting a remote kernel, but it also exists when clearing results.

Description:

When starting a new remote kernel, any results in an existing remote kernel are deleted.

peek 2018-12-18 16-48

Steps to Reproduce:

  1. Connect to remote kernel in first file; create some sort of results.
  2. Connect to remote kernel in second file; results in first file will be gone.

Versions:

Both host and remote computers are running Linux.

Atom    : 1.33.0
Electron: 2.0.11
Chrome  : 61.0.3163.100
Node    : 8.9.3

Plugins:

Do you have any Hydrogen plugins installed and active?

  • hydrogen-python
  • Hydrogen Launcher
  • Data Explorer
@kylebarron kylebarron changed the title Results in all panes are removed when starting a new remote kernel hydrogen:clear-results removes results from all panes Dec 19, 2018
@kylebarron
Copy link
Contributor Author

I looked into this briefly. These are deleted when store.markers.clear(); is called.

store.markers.clear();

The function removes all markers:

clear() {
this.markers.forEach((bubble: ResultView) => bubble.destroy());
this.markers.clear();
}

The issue is that store.markers is a global object shared among all TextEditors that Hydrogen is active in. So any time store.markers.clear() is called, it removes all markers from all panes.

From my point of view, it would be nice if there was some way to keep track of which TextEditor each bubble is in. What about adding a field editorId to each ResultView object? I assume that a ResultView will never move from one TextEditor to another.

@BenRussert

@BenRussert
Copy link
Member

This is an interesting issue. I agree with you about how it probably should work. Although, it does say "all" 🤔

I'm open to the suggestions you mentioned for fixing this. I think it may be helpful in other areas to store outputs and markers somehow so that they can be more easily tied to the editor they belong to.

@kylebarron
Copy link
Contributor Author

This is really annoying because I often start relatively long-running jobs in one file, then while that's running want to start coding in a new file... But then starting a kernel in the new file clears all the bubbles in the first file so when it's done running I don't see anything!

@camall3n
Copy link

camall3n commented Feb 14, 2019

I'm not sure if this is the same issue, but it seems related.

When the results in multiple panes are on the same line, running either output clears the other. But when they are on different lines, running the output has the expected effect, which is to only update the current pane.

multiple-panes-lines-15

Edit: This also happens even when the two panes aren't visible at the same time (e.g. if one pane is behind the other).

Edit: By the way, this particular example doesn't require remote kernels; both are local kernels.

@kylebarron
Copy link
Contributor Author

🤣 That's really sad!

I haven't touched that part of the code but I'm guessing it comes from here:

clearOnRow(row: number) {
let destroyed = false;
this.markers.forEach((bubble: ResultView, key: number) => {
const { start, end } = bubble.marker.getBufferRange();
if (start.row <= row && row <= end.row) {
this.delete(key);
destroyed = true;
}
});
return destroyed;
}

Marker stores aren't tied to an Editor, which is the point of this issue, but that also means that clearOnRow probably clears any markers on the given row in any Editor!

@kylebarron
Copy link
Contributor Author

Just putting some thoughts down...

  • I checked and each pane is a new TextEditor, which simplifies things so that we only have to keep track of markers within a given TextEditor. (Currently, any two panes, regardless of whether they're the same file, remove a marker at the same row on the other pane).

  • I'm not sure what the best way is to associate a MarkerStore to an Editor...

    I was thinking it might be to have a markerMapping in the Store similar to how there's currently a kernelMapping. The kernelMapping is a mapping from a file path to a Kernel object. Likewise the markerMapping could be a mapping from an Editor to a MarkerStore...

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 a pull request may close this issue.

3 participants