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

trashapplet: Remove animation that hasn't worked in years #659

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

lukefromdc
Copy link
Member

@lukefromdc lukefromdc commented Aug 14, 2023

*The trash applet hasn't had working animation in many years
*xstuff_zoom_animate also appears in mate-panel with 4 arguments instead of 2.
*conflicting function names caused a segfault when built in-process

@lukefromdc
Copy link
Member Author

Previously we fixed one segfault (in trash) by excluding the conflicting function from in-process builds of the trash applet. With this we remove any potential segfaults caused by any other applet of the panel calling the wrong function. Why the conflicting functions ever worked I do not know, wondering if the older glib version somehow allowed the applet's version of xstuff_zoom_animate() instead of the panel's function of the same name to be called first.

This was not a glib bug, but rather a bug in our code revealed by a glib update, as it should never have worked and it it did was working by chance only.

@lukefromdc lukefromdc requested a review from a team August 14, 2023 00:09
@lukefromdc lukefromdc changed the title trashapplet: do not use same name for animation mate-panel uses trashapplet: do not use same name for animate function mate-panel uses Aug 14, 2023
@muktupavels
Copy link
Contributor

This will work but if you want avoid similar problems in future then read documentation for g_module_open.

P.S. You don't need to change name of static function.

@raveit65
Copy link
Member

raveit65 commented Aug 14, 2023

The crash is gone with using the internal zoom-animation.
The animation itself do not work (no animation will displayed), same when building out-of-process.
But this is another issue not related to PR.

@lukefromdc
Copy link
Member Author

The question is do we want to keep the currently non-working animation around in case someone comes up with a fix for it later, or remove it as it hasn't worked in years. The latter would allow removal of xstuff.c and xstuff.h from the applet's source files, but would complicate any later rewrite aimed at making the trash icon respond to being clicked on.

@raveit65
Copy link
Member

raveit65 commented Aug 14, 2023

+1 for removing. I don't think that someone will fix it because nobody knows that the function exists.
And it is easy to restore with a git revert.

@lukefromdc
Copy link
Member Author

OK, that's what I will do

*The trash applet hasn't had working animation in many years
*xstuff_zoom_animate also appears in mate-panel with 4 arguments instead of 2.
*conflicting function names caused a segfault when built in-process
@lukefromdc lukefromdc force-pushed the trash-function-name-collision branch from bace53c to 58e1cac Compare August 14, 2023 17:13
@lukefromdc lukefromdc changed the title trashapplet: do not use same name for animate function mate-panel uses trashapplet: Remove animation that hasn't worked in years Aug 14, 2023
@lukefromdc
Copy link
Member Author

Done, simple revert if ever needed

Copy link
Member

@raveit65 raveit65 left a comment

Choose a reason for hiding this comment

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

LGTM

@raveit65 raveit65 merged commit 892cd58 into master Aug 14, 2023
3 checks passed
@raveit65 raveit65 deleted the trash-function-name-collision branch August 14, 2023 20:47
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

3 participants