Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove animations from progress toolbar control #141

Conversation

vogella
Copy link
Contributor

@vogella vogella commented Jun 14, 2022

No description provided.

@vogella vogella force-pushed the remove-animations-from-progress-toolbar-control branch from 30ab5ae to f315610 Compare June 15, 2022 08:07
@vogella vogella requested a review from jukzi June 15, 2022 08:08
@vogella
Copy link
Contributor Author

vogella commented Jun 15, 2022

@jukzi any concerns?

@laeubi
Copy link
Contributor

laeubi commented Jun 15, 2022

Why are animations bad?

@jukzi
Copy link
Contributor

jukzi commented Jun 15, 2022

Would you mind to share a video/screenshot which animation is affected and why you want to remove it?

@laeubi
Copy link
Contributor

laeubi commented Jun 15, 2022

@jukzi I think it is the small icon on the lower right when there is a running job but not sure...

@akurtakov
Copy link
Member

Animations are not bad per se. Not well written and tested animations are not helping the UI though esp. considering that underlying OSes already "animate" the widget .

@vogella
Copy link
Contributor Author

vogella commented Jun 15, 2022

Why are animations bad?

During startup StandardTrim creates an instance of
TaskBarProgressManager in line 52.

Before this change TaskBarProgressManager supports animation, which
seems useless / overkill
for progress reporting. This animation code also causes exception if you
execute unit tests, as I discovered during a test run of the resource
test suite.

This commit removes the animation logic but keeps the update logic of
updating the progress state of the item. I personal cannot note any
difference in the resulting runtime Eclipse.

@jukzi
Copy link
Contributor

jukzi commented Jun 15, 2022

at the bottom right there is an animated progress (which is most time showing wrong progress):
image

is this change about that?

@vogella
Copy link
Contributor Author

vogella commented Jun 15, 2022

at the bottom right there is an animated progress (which is most time showing wrong progress): image

is this change about that?

yes

@jukzi
Copy link
Contributor

jukzi commented Jun 15, 2022

i'd rather like to get the progress fixed instead of removed. can't be that hard since the task bar shows an appropriate percentage:
image

@vogella
Copy link
Contributor Author

vogella commented Jun 15, 2022

i'd rather like to get the progress fixed instead of removed. can't be that hard since the task bar shows an appropriate percentage: image

Are you planning to do this? I won't fix animations.

@jukzi
Copy link
Contributor

jukzi commented Jun 15, 2022

I agree that the current animation is useless
image
I shows just an animation like when progress is not know - could if show the percentage instead, please?

@jukzi
Copy link
Contributor

jukzi commented Jun 15, 2022

This animation code also causes exception if you
execute unit tests, as I discovered during a test run of the resource
test suite.

please attach the stacktraces.

jukzi
jukzi previously requested changes Jun 15, 2022
Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't see a reason to remove the animation. but if we would remove it the drawing does not need to be rescheduled


if (isAnimated && taskItem != null && !taskItem.isDisposed()) {
if (taskItem != null && !taskItem.isDisposed()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is not animated it does not need to be rescheduled

During startup StandardTrim creates an instance of
TaskBarProgressManager in line 52.

Before this change TaskBarProgressManager supports animation, which
seems useless / overkill
for progress reporting. This animation code also causes exception if you
execute unit tests, as I discovered during a test run of the resource
test suite.

This commit removes the animation logic but keeps the update logic of
updating the progress state of the item. I personal cannot note any
difference in the resulting runtime Eclipse.
@vogella vogella force-pushed the remove-animations-from-progress-toolbar-control branch from f315610 to 1bd6871 Compare June 17, 2022 08:11
@vogella
Copy link
Contributor Author

vogella commented Jun 17, 2022

i don't see a reason to remove the animation. but if we would remove it the drawing does not need to be rescheduled

Done

@vogella vogella closed this Jun 17, 2022
@vogella vogella reopened this Jun 17, 2022
@vogella
Copy link
Contributor Author

vogella commented Jun 17, 2022

Like I explained in the commit message I cannot see a difference in behavior after removing the animation.

Here is a screenshot before the removal if I load for example a target platform:

currenty-l f

Here is the same after the removal of the code:

no-animationi-l f

@vogella
Copy link
Contributor Author

vogella commented Jun 17, 2022

As the visual representation is the same I plan to merge this request soon.

updateImage(null);
}
setProgressState(SWT.DEFAULT);
updateImage(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of the job now? If it does always same, what's the point to schedule it at all?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who uses it amd wher / for what reason it is scheduled now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the job you would not get an moving progress bar.

@laeubi
Copy link
Contributor

laeubi commented Jun 17, 2022

Like I explained in the commit message I cannot see a difference in behavior after removing the animation.

I think that the difference is that you now always have an "INDETERMINATE" progressbar but never showing actual progress (even though as @jukzi mentioned sometime wrong progress).

@vogella vogella merged commit 60c3728 into eclipse-platform:master Jun 17, 2022
@vogella vogella deleted the remove-animations-from-progress-toolbar-control branch June 17, 2022 10:16
@vogella
Copy link
Contributor Author

vogella commented Jun 17, 2022

As people seem not have the time to test (everyone seems to guess how it looks like), I merged, you can compare in the next I-build and see if something criticial is missing now.

@jukzi
Copy link
Contributor

jukzi commented Aug 4, 2022

@vogella The commit does not work as intended: The code was NOT about the wrong progress report image - that is still there. Instead it removed the little overlay icon inside the System status bar indicating the image of the currently running job - for example during manual clean build:
image

=> i ask to revert since https://bugs.eclipse.org/bugs/show_bug.cgi?id=471310 claims it was important for them.
I actually never understood or liked that "binary" overlay image during full build but maybe somebody put useful images there. Actually for example PDE export Plugin does try to setup a meaningful image - but it is actually never shown in the taskbar but only in the Progress view.

Since you removed the code for unnamed exceptions please submit the Stacktrace instead and i can look into it.

@jukzi
Copy link
Contributor

jukzi commented Aug 4, 2022

Also the change removed the working(!) Progress in the task bar.
The not working progress is instead: org.eclipse.ui.internal.progress.ProgressAnimationItem.bar - it does not even try to set a correct progress.

@Bananeweizen
Copy link
Contributor

but maybe somebody put useful images there.

Have a look at the progress bar during any egit operation and be amazed. Unfortunately most projects don't call ProgressService.registerIconForFamily(...), even if they define job families.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants