add toggle-result-bubble command

[t9md]

Code is very rough state, but want to know how team feels about this.

What this PR do?

Add new command hydrogen:toggle-bubble

This command allow manually add/remove bubble(outlet of code evaluation).

• If selection was empty, toggle bubble at current cursor line.
• If executed with selection, toggle bubble at each line of selected lines.
• What toggle means is
  • Add bubble at line if line have no bubble.
  • Remove bubble from line if line have bubble.

Motivation

Currently there is no way to add bubble without evaluating code.
So If I tried to place bubble where I want, I have to execute code, then frequently see duplicate declaration of variable like error in bubble.

e.g. let, const in JavaScript cannot be executed in same scope.

This can be avoided if user super carefully execute code, but I don't want to.

Instead, I want to

• Place bubble before evaluating code.
• Then execute from clean state(restart-kernel-and-re-evaluate-bubbles) and update bubbles.

This workflow have no mess, no duplicate declaration error, I prefer this workflow at least in small code snippet.

TODO

• Use better icon for empty bubble(Just added and not yet evaluated state). I picked zap icon

[lgeiger]

Very cool, thanks for your work! That's super valuable for the workflow introduced in #861 🎉

Use better icon for empty bubble(Just added and not yet evaluated state).

I think the easiest thing would be to add a different icon here that indicates that this is just a placeholder.

[t9md]

Thanks @lgeiger for pointer.

Here are icon candidates for empty-bubble I could find from octicons.
How do you think?

• eye
• pin
• zap( I like this )

[t9md]

Ready to review.

For test spec.
I tried to add spec, but give up.
There is no existing test to actually activating Hydrogen package.
I need to understand how store is populated on after activation, and how it is used in the first place to write correct test, but this is too much for me for now sorry.

[lgeiger]

Awesome! I only have a minor comment.

Yeah, the zap icon looks super cool, though I wonder if pin would be more meaningful. @BenRussert @rgbkrk What do you think?

For test spec.
I tried to add spec, but give up.
There is no existing test to actually activating Hydrogen package.
I need to understand how store is populated on after activation, and how it is used in the first place to write correct test, but this is too much for me for now sorry.

Thanks for looking into adding specs! I have a few improvements cued up, so we'll be able to move more logic out of main.js this should greatly help with testing.

[lgeiger]

What about just using clearBubblesOnRow here?

[t9md]

I wanted to know the result status(destroyed of not), but it not return result status.

I could write isRowContainsBubbke(row) predicate function to determine add or remove bubble, I just take simple imperative one.

[lgeiger]

Oh overlooked it, nevermind. I shouldn't review code on my phone 😄

[t9md]

Thanks for quick review and merging!!

[BenRussert]

Thanks for looking into adding specs! I have a few improvements cued up, so we'll be able to move more logic out of main.js this should greatly help with testing.

@lgeiger +1 for moving logic out of main (and of course better testing) which is still open on #657 roadmap!

I think it will help new contributors to look at a clean main to understand program flow first at a high level. Then as a guide to find the functionality they are interested in!

[rgbkrk]

Cool!

[mattiasarro]

This feature seems to have disappeared with some hydrogen update, or is it just me?

[wadethestealth]

No it's not you it's gone see #1642 @mattiasarro