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

Fix startup animation #2166

Merged
merged 2 commits into from
Mar 28, 2024
Merged

Conversation

vanvugt
Copy link
Collaborator

@vanvugt vanvugt commented Mar 21, 2024

  1. Make STARTUP_ANIMATION_TIME non-zero again.
  2. Stop trying to fully replace the startup animation when we can just use the existing one upstream gnome-shell provides.
  3. Slide the dock in when upstream's startup animation is done.

Closes: #2110 and https://bugs.launchpad.net/bugs/2058468

Because gnome-shell stopped exporting the value in 45.0 it was being
treated as undefined, and therefore we had no startup animation.
Rather than trying to duplicate it. And when the upstream animation is
done we can run our own slide-in animation.

Closes: https://bugs.launchpad.net/bugs/2058468
Closes: micheleg#2110
@vanvugt
Copy link
Collaborator Author

vanvugt commented Mar 21, 2024

Tested on both GNOME 46 and 45, which conveniently are the only versions we have to support now.

Copy link
Collaborator

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

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

Thanks, I'm happy to drop this code that we needed to address some issues, I'm wondering if upstream code is good enough to handle the various races we had before though...

Comment on lines -2215 to -2242
if (Main.layoutManager._startingUp && Main.layoutManager._waitLoaded) {
// Disable this on versions that will include:
// https://gitlab.gnome.org/GNOME/gnome-shell/-/merge_requests/2763
this._methodInjections.addWithLabel(Labels.STARTUP_ANIMATION,
Main.layoutManager.constructor.prototype,
'_prepareStartupAnimation', function (originalMethod, ...args) {
/* eslint-disable no-invalid-this */
const dockManager = DockManager.getDefault();
const temporaryInjections = new Utils.InjectionsHandler(
dockManager);

const waitLoadedHandlingMonitors = (_, bgManager) => {
return new Promise((resolve, reject) => {
const connections = new Utils.GlobalSignalsHandler(
dockManager);
connections.add(bgManager, 'loaded', () => {
connections.destroy();
resolve();
});

connections.add(Utils.getMonitorManager(), 'monitors-changed', () => {
connections.destroy();

reject(new GLib.Error(Gio.IOErrorEnum,
Gio.IOErrorEnum.CANCELLED, 'Loading was cancelled'));
});
});
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally I'd be very happy to drop all this code, but one reason for that was that https://gitlab.gnome.org/GNOME/gnome-shell/-/merge_requests/2763 was still a thing at the time.

I've not tested anymore the script, but that was triggering the same root issue we had that was leading to a black/non-responsive screen input area.

So, have you tested with tricks like those and see if we're strong enough now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't remember all the details of those clipping bugs, only that it happened twice in the past. So we should investigate, but I am keen to build on a much smaller code base if any new fix is required. I vaguely recall finding upstream had the same bug, so that plus the fact that we no longer modify the upstream animation, should mean any fix required for the clipping issue could go upstream.

Copy link
Collaborator Author

@vanvugt vanvugt Mar 26, 2024

Choose a reason for hiding this comment

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

Sorry for the delay...

The X11 VM clipping bug does still happen upstream (no extensions), but only for logging into Main.sessionMode.hasOverview == false which is not the default. And the clipping gets corrected at the end of the login animation.

dash-to-dock master does not have the bug, but to test that you also need to fix the undefined STARTUP_ANIMATION_TIME issue or else there's no way to slow down animations.

dash-to-dock #2166 does have the bug, but it now corrects itself at the end of the login animation just like upstream does.

So we no longer need to carry the clipping fix here. The bug is now the same with or without extensions after this PR. It should be treated as an upstream bug (when Main.sessionMode.hasOverview == false) from now on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So I checked the script and the bug is back... mhmh

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that's kind of what I said in the last two paragraphs. I think it's OK. It recovers at the end of the animation and the same bug occurs without extensions so any fix should go in upstream gnome-shell.

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.

JS ERROR: TypeError: this._prepareStartupAnimation() is undefined
2 participants