Closed MikeInnes closed 8 years ago
Nice!
Looks good! Couple of things:
opacity: 0.5
or so) when nothing is happening just so it doesn't look like a new element is added.Yup, tests are good, we should be more vigilant about that.
Right now the indefinite progress bar moves for a little while and then stops. If that weren't the case I'd use the stopped progress bar for "not working", but I don't think we have control over it. We could perhaps think of some other visual to use as a placeholder.
I like your idea but my concern is that we're working around too many APIs – the progress API, the status bar API etc, which for one thing makes playing nicely with themes more difficult. Worth avoiding that if possible. Let me know if you have any other thoughts on it though.
I don't think the problems are too immediate, I'll merge this so that we can build on it. Thanks for the review!
Right now the indefinite progress bar moves for a little while and then stops. If that weren't the case I'd use the stopped progress bar for "not working", but I don't think we have control over it. We could perhaps think of some other visual to use as a placeholder.
I just fixed that in https://github.com/JunoLab/atom-ink/commit/6cf13ff0401770176c729dc1707ea92b2a5fc773, but was thinking about using a semitransparent empty progress meter as default anyways.
Also, I think we should extend the API to support something like done()
, which doesn't delete the tile but rather hides (or makes it transparent or whatever) the progress bar -- seems a bit strange to me to always delete the progress bar and the tile when something has finished. Maybe also rename push
to add
, because it doesn't unconditionally push a new element to metres
.
Also also, it'd probably be valuable to write a few devdocs for new features we add -- I think it helps a lot with designing (and reviewing) APIs.
using a semitransparent empty progress meter as default anyways.
Yeah that makes a lot of sense doesn't it – probably should have thought of that :)
My only concern then would be that it's nice to have Ink/JuliaClient stay out of the way if you're not using it – we don't want to take over the editor. But we can perhaps defer adding the progress bar til it's first needed to address that. Will make these changes.
it doesn't unconditionally push a new element to metres.
Well, it doesn't unconditionally add one either. I think the semantics are OK as long as we think of metres
as an ordered set (Julia has push!(set, x)
too), but I'm not too worried, add
is fine also. I think we use push
elsewhere though.
I will throw together some devdocs as well.
Yeah, I was thinking of just adding the metre as soon as it's required the first time and then staying there until it isn't anymore (which will probably only happen if the package is uninstalled, unless we want the progress metre to be buffer-dependent (which is a bad idea)).
About the push
stuff: Point taken :)
I think I just don't quite understand how all of this is supposed to work when there are multiple progress metres at any one time -- it seemed like metres
would then behave like an ordered array... I'll just wait for the devdocs, should hopefully be clearer then.
It will make more sense once we have the rest of the UI, because the stack in metres
will reflect the stack you see when you hover over the progress bar. I'm trying to design it so that it makes sense even without the stack and you can hover for more info when needed – but I'll write this up a bit anyway.
Just an idea: Could we have a "global" progress bar, which is the union of all progress bars in the stack? It is indeterminate if all of them are, and otherwise displays the cumulative progress. Hovering would then give you the components plus their origin (or names or whatever).
You'd have to figure out how to weight them. e.g. say you have something like:
upload() = @progress for file in files ... @progress for bytes in file
You could do cumulative progress if you knew that e.g. each file accounts for 10% of the overall progress, but equal weighting is going to make the bar jump up and down.
The stack design is meant to address this in a "good enough" way, since short-lived operations will disappear from the status bar while longer-running things will stick around, so you only see overall progress. And it should also work ok for things where you have multiple threads making progress on independent operations and so on.
This eschews the in-editor spinner for a (possibly indefinite) progress metre in the status bar:
This is more composable between packages, and in future I plan to have some UI for multiple/nested progress metres. But more interestingly it lets us do stuff like this:
cc @davidanthoff