Add setting to view output in dock by default #1022
Conversation
lib/main.js
Outdated
| if (!atom.config.get("Hydrogen.outputAreaDefault")) return; | ||
| openOrShowDock(OUTPUT_AREA_URI); | ||
| } | ||
| ); |
There was a problem hiding this comment.
The string argument here is a "debug name" that will appear in the log when the reaction fires (only in dev mode when mobx-react-devtools enabled.
lib/utils.js
Outdated
| : atom.workspace.open(URI, { | ||
| searchAllPanes: true, | ||
| activatePane: false | ||
| }); | ||
| } |
There was a problem hiding this comment.
The show part should be last so it opens if not already opened, then shows the dock workout focusing it. I'll rebase this PR later.
|
Thanks for tackling this. I just tested it and unfortunately it doesn't fully work. When first starting a new kernel it opens the dock but the output is still displayed inline. Do we really need the |
|
The reason for the when is that it would only open in the pane when you start up the first kernel. Following that it would work as it does now where you can switch to either inline or pane output. The very first output is displaying inline, trying to fix that. I'll try your suggestion, I guess we would toggle the setting when the pane disposes? Otherwise you could never use inline if the setting is still checked. Thanks for the feedback! |
BenRussert
force-pushed
the
output-pane-config
branch
from
9493184
to
7d5d29a
Compare
lib/main.js
Outdated
| const globalOutputStore = atom.workspace | ||
| .getPaneItems() | ||
| .find(item => item instanceof OutputPane) | ||
| const globalOutputStore = atom.config.get("Hydrogen.outputAreaDefault") | ||
| ? kernel.outputStore | ||
| : null; |
There was a problem hiding this comment.
I think this should be
const globalOutputStore =
atom.config.get("Hydrogen.outputAreaDefault") ||
atom.workspace.getPaneItems().find(item => item instanceof OutputPane)
? kernel.outputStore
: null;since we still want to have the possibility of just opening the output pane and using it until it's completely closed.
lib/panes/output-area.js
Outdated
| @@ -15,6 +15,8 @@ export default class OutputPane { | |||
| constructor(store: store) { | |||
| this.element.classList.add("hydrogen"); | |||
|
|
|||
| atom.config.set("Hydrogen.outputAreaDefault", true); | |||
There was a problem hiding this comment.
Why are you setting the config value here?
There was a problem hiding this comment.
With the last commit we don't need to. Originally it was so that if you toggle open the output pane the setting would turn on.
There was a problem hiding this comment.
Is there follow up that needs to be done here?
|
It looks like all the feedback was addressed, we now need a rebase and then it's time to bring it in. |
Off when clearing output On when toggle opens a new pane instance
BenRussert
force-pushed
the
output-pane-config
branch
from
3b71908
to
fcc28e8
Compare
|
Now I seem to be opening multiple output panes? I might have to look at this again tomorrow. |
This reverts commit d957c2f.
|
I think this is good now.
I got caught up wanting an intuitive way to switch back to inline even if you have the setting checked, but that is probably unnecessary or at least can just come back to that if people want it later. |
There was a problem hiding this comment.
I just gave this a quick spin and haven't found any problems 👍
@BenRussert Thanks for keeping this PR alive over this long period.
Yeah I think it's unnecessary. People can switch between both views very nicely when the setting is disabled. If it's enabled they'll never see the inline bubbles, which I think is OK. |
In order to do this for #995 I reworked the logic for opening watches/output panes a little bit.