Closes #92135 - Adds view-level progress indicator #92136
Conversation
There was a problem hiding this comment.
@eamodio can you please find someone from layout team to review the progress-unrelated changes like @sandy081 or @sbatten .
As for the files processIndicator.ts and progressService.ts I see the new code is pretty much a copy of existing code. Can we avoid code duplication and extract pieces into reusable things that can be reused?
PS: I am not sure about the impact for when views can be put into the panel, but I would suggest to check on that as well.
|
@bpasero @sbatten worked with me on the changes for 👍 Will look more into removing duplication. As for the views in the panel, I guess it will depend on how they will be added there -- if they have the header it should still work, but depending on the layout, we might want some custom css to move/change the indicator when its in the panel. |
|
@bpasero I pushed changes to consolidate |
There was a problem hiding this comment.
@eamodio ok I ignored changes outside of the progress area.
Overall seems ok to me with some comments. I find hideWhenInactive a bit weird, why does that need to be conditional? Should we always hide when inactive or does this concept not exist in other parts?
src/vs/workbench/services/progress/test/browser/progressIndicator.test.ts
Show resolved
Hide resolved
|
I'm honestly not sure why we don't always hide in the places where I added the conditional. I think it is related to the fact that in the existing usages the progress bar is a shared among all the viewlet's, where the new view one is attached to each view. But I didn't want to break any existing behavior by changing that. And without hiding it for the view case you end up with progress that won't end if the view get hidden (collapsed) while running. |
|
@bpasero let me know if this can be merged now -- thanks! |
|
@eamodio conflicts |
eamodio
force-pushed
the
eamodio/view-progress
branch
from
a044743
to
0daa3f3
Compare
|
@bpasero I've resolved the conflicts with |
| @@ -194,17 +198,23 @@ export class CompositeProgressIndicator extends CompositeScope implements IProgr | |||
| progressbar: ProgressBar, | |||
| scopeId: string, | |||
| isActive: boolean, | |||
| private readonly options: { hideWhenInactive?: boolean } | undefined, | |||
There was a problem hiding this comment.
@eamodio everything is fine for me, but this option is impossible to understand, because it is in fact not just used to hide the progress when the view becomes deactivated but also seems to be used in other places. since I do not understand it and do not really like it, I am against this change because it makes this component harder to use. please find an alternative maybe by aligning all progress indicators to behave the same.
There was a problem hiding this comment.
The (poorly named) option, is basically a flag to signal that the progress bar for the indicator is shared or not. If it is shared (e.g. viewlet), then we only want to hide the progress if it is active. If it is not shared (e.g. view), then we want to stop it in any of those cases.
There was a problem hiding this comment.
Maybe a better name is hideEvenWhenInactive or exclusiveProgressBar
There was a problem hiding this comment.
@eamodio how is the progress indicator for a view different for a viewlet or panel? I do not understand it nor the concept of a shared indicator.
My understanding is that a progress indicator has a scope (view id, viewlet id or panel id) and only reacts on events matching this ID. So if a viewlet changes, we update the progress bar in the same way I would think we should do it for a view. How is a view different?
If at all we do figure out that we need to have different behaviour, maybe we could have a base class for the indicator and then 2 specific cases: shared and non-shared indicator. But again, I see no reason yet to go there.
There was a problem hiding this comment.
#92136 (comment) seems relevant, and we need to understand it better I guess.
There was a problem hiding this comment.
The root difference is that all viewlets share a single progress bar, since its always in the same location at the top of the sidebar and there can only be one progress showing at a time. Where as the view each have their own progress bar which call have multiple showing at any time.
There was a problem hiding this comment.
@eamodio I get that and still: when the viewlet is inactive, I want the progress to stop. same as I would expect that for a view that is hidden or collapsed.
There was a problem hiding this comment.
Not really, because the viewlet that became active could have running progress which will now use that same progress bar, so if we stopped it, it could interfere with it. But again, I didn't create, nor change that behavior -- that was the existing behavior. I just needed to adapt it because that doesn't work for the view case where there are independent progress bars.
Consolidates ViewProgressIndicator into CompositeProgressIndicator
eamodio
force-pushed
the
eamodio/view-progress
branch
from
0daa3f3
to
da6caf5
Compare
eamodio
force-pushed
the
eamodio/view-progress
branch
from
da6caf5
to
58af7d4
Compare
This PR fixes #92135
This adds view pane support to
ProgressService.withProgress. Here is it in use with the Timeline view: