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

mate-base/mate-settings-daemon: Fix incompatible function pointer types #31624

Closed
wants to merge 1 commit into from

Conversation

listout
Copy link
Contributor

@listout listout commented Jun 26, 2023

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @listout
Areas affected: ebuilds
Packages affected: mate-base/mate-settings-daemon

mate-base/mate-settings-daemon: @gentoo/mate

Linked bugs

Bugs linked: 881315


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Jun 26, 2023
@listout
Copy link
Contributor Author

listout commented Jun 26, 2023

Sending patch upstream

@@ -0,0 +1,14 @@
Bug: https://bugs.gentoo.org/881315
Copy link
Member

Choose a reason for hiding this comment

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

You've sent it upstream, I assume? If so, where's the link? If not, why not? 😉

Always upstream!

atspi_init ();

- self->listener = atspi_device_listener_new (on_key_press_event,
+ self->listener = atspi_device_listener_new ((AtspiDeviceListenerCB)on_key_press_event,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not yet sure if this is actually right. I need to check.

Copy link
Contributor Author

@listout listout Jun 26, 2023

Choose a reason for hiding this comment

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

My initial patch was to remove const from on_key_press_event's paramer

 static gboolean
-on_key_press_event (const AtspiDeviceEvent *event,
+on_key_press_event (AtspiDeviceEvent *event,
                     void                   *user_data G_GNUC_UNUSED)
 {
         /* don't ring on capslock itself, that's taken care of by togglekeys

Do you think it's more appt. than the casting?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for asking - I think the patch as it is (the cast) is better now I looked at it harder.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-06-26 17:30 UTC
Newest commit scanned: ad5d9c1
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/60c09bcf2c/output.html

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-06-26 18:40 UTC
Newest commit scanned: 685bda5
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/fb91299776/output.html

@listout
Copy link
Contributor Author

listout commented Jun 27, 2023

Maintainer disagrees with the patch and will probably fix it themselves
mate-desktop/mate-settings-daemon#399 (comment)

@thesamesam
Copy link
Member

thesamesam commented Jun 27, 2023

Thanks. This is a good example of why:

  1. upstreaming is important;
  2. casting should not be the first thing one does if we can help it (because it'll always "work" to silence the warning even if it's wrong)

@listout
Copy link
Contributor Author

listout commented Jun 27, 2023

I'll update the patch to match that of maintainers.

@listout
Copy link
Contributor Author

listout commented Jun 27, 2023

Huh, in the upstream correct PR by dev, they are just removing the const parameter from the function, mate-desktop/mate-settings-daemon@42e91f2.

@thesamesam
Copy link
Member

Yeah, you can see they had to think for a while about whether it's the best way to go though and that it's a trade-off (and the API changed).

The patch also fixes a memory leak, take from an open PR by mate dev.

Closes: https://bugs.gentoo.org/881315
Signed-off-by: Brahmajit Das <brahmajit.xyz@gmail.com>
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-06-27 17:15 UTC
Newest commit scanned: b0ab963
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/918349a2f2/output.html

MocioF pushed a commit to MocioF/gentoo that referenced this pull request Aug 23, 2023
The patch also fixes a memory leak, take from an open PR by mate dev.

Closes: https://bugs.gentoo.org/881315
Signed-off-by: Brahmajit Das <brahmajit.xyz@gmail.com>
Closes: gentoo#31624
Signed-off-by: Joonas Niilola <juippis@gentoo.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR.
Projects
None yet
5 participants