search

JunoLab / atom-ink

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

Progress refactoring take 2 #90

Closed pfitzseb closed 7 years ago

pfitzseb commented 7 years ago

supersedes #76.

pfitzseb commented 7 years ago

Alright, this should be good to go now. Only thing we maybe should think about is displaying arbitrary HTML instead of strings to the left and right of the progress bars in the stack, which would be more future proof (in case we want buttons or stuff like that in there).

Also, I'd appreciate you having a look at this, @MikeInnes.

MikeInnes commented 7 years ago

Looking good so far but I think we should consider the interface we're exposing carefully so that it's as good as possible for other clients.

Right now my impression is that the API is convenient for clients that are very similar to ours (funny coincidence that eh) but not that idiomatic compared to other Atom APIs. We should definitely err towards the latter, even if it means julia-client doing a little extra coordination work.

My suggestion would be something like the following:

p = progress.create() # adds a new bar and returns an object representing it
p.update(0.5) # set value
p.update() # set indeterminate
p.setText("foo")
p.destroy()

If we use the object to refer to a given bar we don't need IDs. Also, ink should probably not export an empty method since the progress meter may be shared between language clients. Instead the client can keep a list of bars it created and destroy those.

pfitzseb commented 7 years ago

Alright, I changed the API to something more idiomatic. Let me know what you think, and thanks for the feedback!

Ah, and regarding IDs: While we don't need them in ink with this approach, the communication between julia-client and julia still needs them.

MikeInnes commented 7 years ago

I've tweaked this to show indeterminate progress bars. Feel free to raise any major objection to that, but I think this makes sense for user-defined bars and when multiple language clients are involved. The main downside is showing a (usually redundant) Julia bar, but we can figure something out there if it becomes annoying.

pfitzseb commented 7 years ago

Regarding the mostly unnecessary Julia progress bar: We could extend the ink API with a ProgressBar.hidden attribute, which would keep a given progress bar from being displayed in the stack. I don't think that's crucial though.

MikeInnes commented 7 years ago

Let's see how it plays out for now. We can change things if it's annoying but for now it's a simple way to handle the multi-client case, and anything else makes that more complex.