JunoLab / atom-ink

IDE toolkit for Atom
MIT License
229 stars 40 forks source link

WIP: Make plotpane pannable and zoomable #139

Closed pfitzseb closed 7 years ago

pfitzseb commented 7 years ago

for "dumb" plots (i.e. all plots that aren't a webview, like plotly).

pfitzseb commented 7 years ago

Icons! And it actually mostly works: zoomcontrols This now requires https://github.com/JunoLab/atom-julia-client/pull/355 on the julia-client side.

cc @cormullion

cormullion commented 7 years ago

I want it now! Well, OK, I'll wait for a few days... :)

MikeInnes commented 7 years ago

Neat idea. I think the design would be cleaner if the plot pane didn't special-case this behaviour; it seems like it should be possible to hand the plot pane a Pannable and have it figure out the right buttons and such using the current interfaces. It's already possible for the plot itself to customise the toolbar, for example.

Can you pull out the trivial stuff (cleanups, semicolons) so that the diff is a bit clearer?

pfitzseb commented 7 years ago

Hm, I guess the question is what we want the ink API to be: I tried to keep all of the etch stuff out of it, which means the API only consists of some plotpane methods like show and update.

And then we can't very well have some heuristic in ink for deciding which toolbar buttons to show. We probably could redesign the ink side a bit to make that possible though...

MikeInnes commented 7 years ago

We don't need any heuristics; I forget exactly what the API is now, but there's something like a getToolbar() method that the plot pane will call on the given item, which allows the item to show custom buttons. This is how the profiler works currently.

pfitzseb commented 7 years ago

Right, that's not what I meant (though my explanation sucks) :D

In julia-client we show a plot by creating a PlotPane and then calling show on it. And the decision whether to show a toolbar must be made at that point, since the argument to show will just be some HTML or DOM object or whatever.

Now we could of course wrap views.render view in a Pannable and get the correct behaviour, but that would mean that ink would have to make Pannable part of the public API. I'm not convinced that's a good idea, but if you think so I'm happy to change the implementation.

MikeInnes commented 7 years ago

Right, yeah, so julia-client would then be deciding what is pannable, and calling pane.show(new Pannable(item). That seems fairly sensible to me; pannable-ness is an independently useful concept, and we're reducing what the plot pane is responsible for. In general I think that being able to compose independent things together is much nicer than having lots of keyword arguments.

Only tricky thing is that you'd then want Pannable to forward toolbar to its sub-item. toolbar should probably be a function, but otherwise that shouldn't be too tricky.

I agree that etch shouldn't be part of the interface, but as long as Pannable is happy to work with dom objects then etch remains an implementation detail.

pfitzseb commented 7 years ago

Sounds good to me -- I'll reimplement this then ;)

pfitzseb commented 7 years ago

Alright, this should be much cleaner now. :) Also actually works pretty well now.

I'd appreciate you having another look, but I think this should be good to go.