After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 782549 - Bugs in #781572 porting to C
Bugs in #781572 porting to C
Status: RESOLVED FIXED
Product: gnome-games
Classification: Core
Component: general
3.24.x
Other Linux
: Normal normal
: ---
Assigned To: GNOME Games maintainers
GNOME Games maintainers
Depends on:
Blocks:
 
 
Reported: 2017-05-12 09:12 UTC by Abhinav Singh
Modified: 2017-05-14 10:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gamepad: Save changes made to the DPad's state (2.17 KB, patch)
2017-05-12 19:56 UTC, Adrien Plazas
committed Details | Review
gamepad: Fix NULL checks in gamepadmapping destructor (1.09 KB, patch)
2017-05-12 19:56 UTC, Adrien Plazas
committed Details | Review
gamepad: Fix empty string test (995 bytes, patch)
2017-05-12 19:56 UTC, Adrien Plazas
committed Details | Review
gamepad: Fix global includes (848 bytes, patch)
2017-05-12 19:56 UTC, Adrien Plazas
committed Details | Review
gamepad: Fix local includes (975 bytes, patch)
2017-05-14 09:14 UTC, Abhinav Singh
none Details | Review
gamepad: Fix local includes (1022 bytes, patch)
2017-05-14 10:05 UTC, Abhinav Singh
committed Details | Review

Description Abhinav Singh 2017-05-12 09:12:12 UTC
A few things were broken, and a few memory leaks were introduced during the https://bugzilla.gnome.org/show_bug.cgi?id=781572 ports to C.

This bug is a collection for all porting bugs for 781572.
Comment 1 Abhinav Singh 2017-05-12 09:38:45 UTC
https://git.gnome.org/browse/gnome-games/tree/src/gamepad/gamepad-mapping.c

#316: NULL check instead of empty check
#440 #442 #444: NULL checks are inverted
#7 #8: local includes
Comment 2 Abhinav Singh 2017-05-12 09:57:44 UTC
https://git.gnome.org/browse/gnome-games/tree/src/gamepad/gamepad-monitor.c

#7 #8 #9 #10: global includes
Comment 3 Adrien Plazas 2017-05-12 19:43:16 UTC
In gamepqd-mapping.c in the games_gamepad_mapping_get_dpad_mapping() function, a copy of the DPad object is made into the dpad variable, we want the changes to be applied on the original hence a reference (pointer) would work better here.
Comment 4 Adrien Plazas 2017-05-12 19:56:08 UTC
Created attachment 351752 [details] [review]
gamepad: Save changes made to the DPad's state

Take a reference to the DPad object rather than making a copy in
games_gamepad_mapping_get_dpad_mapping() so that the cahnges or saved.
Comment 5 Adrien Plazas 2017-05-12 19:56:13 UTC
Created attachment 351753 [details] [review]
gamepad: Fix NULL checks in gamepadmapping destructor

We want to free non-NULL pointers, not NULL ones.
Comment 6 Adrien Plazas 2017-05-12 19:56:20 UTC
Created attachment 351754 [details] [review]
gamepad: Fix empty string test

Test if a string is empty instead of NULL as it was the intended test in
games_gamepad_mapping_new_from_sdl_string().
Comment 7 Adrien Plazas 2017-05-12 19:56:25 UTC
Created attachment 351755 [details] [review]
gamepad: Fix global includes

Use <> for some global includes instead of "".
Comment 8 Adrien Plazas 2017-05-13 09:40:20 UTC
Attachment 351752 [details] pushed as dfba6a5 - gamepad: Save changes made to the DPad's state
Attachment 351753 [details] pushed as 18d6836 - gamepad: Fix NULL checks in gamepadmapping destructor
Attachment 351754 [details] pushed as 91fd301 - gamepad: Fix empty string test
Attachment 351755 [details] pushed as d5190e8 - gamepad: Fix global includes
Comment 9 Abhinav Singh 2017-05-14 09:14:19 UTC
Created attachment 351808 [details] [review]
gamepad: Fix local includes

Use "" for some local includes instead of <>.
Comment 10 Adrien Plazas 2017-05-14 09:24:04 UTC
Review of attachment 351808 [details] [review]:

Thanks, though there is an error and we could fix things further. :)

::: src/gamepad/gamepad-monitor.c
@@ +5,3 @@
 #include <glib.h>
 #include <glib-object.h>
+#include "linux-raw-gamepad-monitor.h"

Shouldn't it be "linux/linux-raw-gamepad-monitor.h" instead? It worked with <> because the Linux dir is in the include paths.

@@ +8,3 @@
+#include "raw-gamepad-monitor.h"
+#include "raw-gamepad.h"
+#include "gamepad-mapping-error.h"

Let's take it as an occasion to move this one first to sort them alphabetically.
Comment 11 Abhinav Singh 2017-05-14 10:05:50 UTC
Created attachment 351810 [details] [review]
gamepad: Fix local includes

Use "" for some local includes instead of <>, and reorder the includes
alphabetically.
Comment 12 Adrien Plazas 2017-05-14 10:14:49 UTC
Attachment 351810 [details] pushed as b4cfcc9 - gamepad: Fix local includes