GNOME Bugzilla – Bug 780754
Let users setup their gamepads
Last modified: 2017-08-07 19:50:07 UTC
We should add some way to let the users set up their gamepads so they can use them with Games.
Created attachment 354351 [details] [review] gamepad: Add GamepadInput vapi This will be used for building the gamepad mappings.
Created attachment 354352 [details] [review] gamepad: Add 'name' & 'guid' properties to Gamepad These will be used to build mappings for gamepads in subsequent commits.
Created attachment 354353 [details] [review] ui: Add basic preference screen for inputs Currently, it shows the gamepad names and their orders. This screen will allow the user to do inputs related settings.
Created attachment 354354 [details] [review] gamepad: Add 'unplugged' signal to GamepadMonitor The motivation is that without this signal, the users of GamepadMonitor were forced to maintain an array for the plugged-in gamepads.
Created attachment 354355 [details] [review] ui: Allow real time gamepad list for gamepad pref Update the list on 'gamepad-plugged' and 'gamepad-unplugged' signals from the GamepadMonitor.
Created attachment 354356 [details] [review] gamepad: Add standard-gamepad.svg This image will be used to guide the users through gamepad mapping.
Created attachment 354357 [details] [review] gamepad: Add GamepadInput vapi This will be used for building the gamepad mappings.
Created attachment 354358 [details] [review] gamepad: Add get_standard_inputs() to GamepadInput This will ease getting standard inputs in users of this class.
Created attachment 354359 [details] [review] gamepad: Add 'name' & 'guid' properties to Gamepad These will be used to build mappings for gamepads in subsequent commits.
Created attachment 354360 [details] [review] ui: Add basic preference screen for inputs Currently, it shows the gamepad names and their orders. This screen will allow the user to do inputs related settings.
Created attachment 354361 [details] [review] gamepad: Add 'unplugged' signal to GamepadMonitor The motivation is that without this signal, the users of GamepadMonitor were forced to maintain an array for the plugged-in gamepads.
Created attachment 354362 [details] [review] ui: Allow real time gamepad list for gamepad pref Update the list on 'gamepad-plugged' and 'gamepad-unplugged' signals from the GamepadMonitor.
Created attachment 354363 [details] [review] gamepad: Add standard-gamepad.svg This image will be used to guide the users through gamepad mapping.
Created attachment 354364 [details] [review] gamepad: Add GamepadView & its dependency librsvg This adds the ability to highlight gamepad svg's individual inputs.
Created attachment 354365 [details] [review] gamepad: Add GamepadMappingBuilder This will be used to build the sdl string from user's inputs.
Created attachment 354366 [details] [review] ui: Add GamepadMapper Use GamepadView to get the user input & GamepadMappingBuilder to build the sdl string from those inputs.
Created attachment 354367 [details] [review] ui: Show GamepadMapper on click in gamepad prefs
Created attachment 354368 [details] [review] ui: Allow PreferencesPage to show custom headers This will allow the PreferencesPages to show widgets in the header bar. This is based on the tentative designs at https://wiki.gnome.org/Design/Apps/Settings
Created attachment 354369 [details] [review] ui: Use stack in PreferencesPageInput Showing GamepadMapper as a Dialog resulted in three windows on the screen which is against HIG recommendations. This commit changes the PreferencesPageInput to use stack and GamepadMapper use to box.
Created attachment 354370 [details] [review] ui: Add skip button to GamepadMapper Many gamepads do not possess all the inputs from a standard gamepad, hence the user should be allowed to skip those inputs.
Created attachment 354371 [details] [review] gamepad: Add GamepadDpad vapi file This will help us save the previous state of Dpads in the GamepadMappingsBuilder.
Created attachment 354372 [details] [review] gamepad: Allow more hat mappings in MappingBuilder Save previous hat axis and value to build the position for the current hat press. This way we can support hat to buttons and axis mappings.
Created attachment 354373 [details] [review] ui: Improve error displays in GamepadMapper Make the errors hide automatically and add error messages for incompatible mappings. This commit also fixes minor style problems.
Created attachment 354374 [details] [review] gamepad: Add GamepadMapping vapi This will be used by set_mapping() in Gamepad.
Created attachment 354375 [details] [review] gamepad: Add set_mapping() to Gamepad This will allow us to change mappings of Gamepad even after its construction.
Created attachment 354376 [details] [review] gamepad: Use file uris in GamesMappingsManager This will simplify the code and increase reusability for the subsequent commits.
Created attachment 354377 [details] [review] gamepad: Free Gamepad of GamepadMappingsManager The mapping for Gamepad should be set by GamepadMonitor. Gamepad should not have knowledge about GamepadMappingsManager.
Created attachment 354378 [details] [review] gamepad: Separate mappings in GamepadMappingsManager This will allow querying the default mappings or the user mappings for a gamepad separately.
Created attachment 354379 [details] [review] gamepad: Allow MappingsManager to save user mappings This commit adds functionality in GamepadMappingsManager to allow it to save user mappings in user's config directory.
Created attachment 354380 [details] [review] gamepad: Add GamepadMappingsManager vapi This will be used to save the sdl_string in GamepadMapper.
Created attachment 354381 [details] [review] ui: Allow GamepadMapper to save/reset mappings Add reset button to the GamepadMapper, and allow it to access GamepadMappingsManager to reset and save user mappings.
Review of attachment 354358 [details] [review]: ::: src/gamepad/gamepad-input.c @@ +38,3 @@ + +GamesGamepadInput * +games_gamepad_input_get_standard_inputs (int *length) This really looks like it should be a constant: you are creating copies of this, your store them later just to read their constant: what about having a constant array of constant inputs in Vala instead? Also you only use this related to mappings, so this array isn't a property of the InputType itself but of the mappings, hence I would recommend to move that code elsewhere (allowing you to write it in Vala).
Review of attachment 354357 [details] [review]: LGTM.
Review of attachment 354359 [details] [review]: LGTM.
Created attachment 355142 [details] [review] ui: Add basic preference screen for inputs Currently, it shows the gamepad names and their orders. This screen will allow the user to do inputs related settings.
Created attachment 355143 [details] [review] gamepad: Add 'unplugged' signal to GamepadMonitor The motivation is that without this signal, the users of GamepadMonitor were forced to maintain an array for the plugged-in gamepads.
Created attachment 355144 [details] [review] ui: Allow real time gamepad list for gamepad pref Update the list on 'gamepad-plugged' and 'gamepad-unplugged' signals from the GamepadMonitor.
Created attachment 355145 [details] [review] gamepad: Add standard-gamepad.svg This image will be used to guide the users through gamepad mapping.
Created attachment 355146 [details] [review] gamepad: Add GamepadView & its dependency librsvg This adds the ability to highlight gamepad svg's individual inputs.
Created attachment 355147 [details] [review] gamepad: Add GamepadMappingBuilder This will be used to build the sdl string from user's inputs.
Created attachment 355148 [details] [review] ui: Add GamepadMapper Use GamepadView to get the user input & GamepadMappingBuilder to build the sdl string from those inputs.
Created attachment 355149 [details] [review] ui: Show GamepadMapper on click in gamepad prefs
Created attachment 355150 [details] [review] ui: Allow PreferencesPage to show custom headers This will allow the PreferencesPages to show widgets in the header bar. This is based on the tentative designs at https://wiki.gnome.org/Design/Apps/Settings
Created attachment 355151 [details] [review] ui: Use stack in PreferencesPageInput Showing GamepadMapper as a Dialog resulted in three windows on the screen which is against HIG recommendations. This commit changes the PreferencesPageInput to use stack and GamepadMapper use to box.
Created attachment 355152 [details] [review] ui: Add skip button to GamepadMapper Many gamepads do not possess all the inputs from a standard gamepad, hence the user should be allowed to skip those inputs.
Created attachment 355153 [details] [review] gamepad: Add GamepadDpad vapi file This will help us save the previous state of Dpads in the GamepadMappingsBuilder.
Created attachment 355154 [details] [review] gamepad: Allow more hat mappings in MappingBuilder Save previous hat axis and value to build the position for the current hat press. This way we can support hat to buttons and axis mappings.
Created attachment 355155 [details] [review] ui: Improve error displays in GamepadMapper Make the errors hide automatically and add error messages for incompatible mappings. This commit also fixes minor style problems.
Created attachment 355156 [details] [review] gamepad: Add GamepadMapping vapi This will be used by set_mapping() in Gamepad.
Created attachment 355157 [details] [review] gamepad: Add set_mapping() to Gamepad This will allow us to change mappings of Gamepad even after its construction.
Created attachment 355158 [details] [review] gamepad: Use file uris in GamesMappingsManager This will simplify the code and increase reusability for the subsequent commits.
Created attachment 355159 [details] [review] gamepad: Free Gamepad of GamepadMappingsManager The mapping for Gamepad should be set by GamepadMonitor. Gamepad should not have knowledge about GamepadMappingsManager.
Created attachment 355160 [details] [review] gamepad: Separate mappings in GamepadMappingsManager This will allow querying the default mappings or the user mappings for a gamepad separately.
Created attachment 355161 [details] [review] gamepad: Allow MappingsManager to save user mappings This commit adds functionality in GamepadMappingsManager to allow it to save user mappings in user's config directory.
Created attachment 355162 [details] [review] gamepad: Add GamepadMappingsManager vapi This will be used to save the sdl_string in GamepadMapper.
Created attachment 355163 [details] [review] ui: Allow GamepadMapper to save/reset mappings Add reset button to the GamepadMapper, and allow it to access GamepadMappingsManager to reset and save user mappings.
Created attachment 355164 [details] [review] gamepad: Add standard-gamepad-inputs This will be used for gamepad mappings.
Review of attachment 355152 [details] [review]: This should be part of the commit adding GamepadMapper.
Review of attachment 355155 [details] [review]: This should be part of the commit adding GamepadMapper.
Review of attachment 355163 [details] [review]: This should be part of the commit adding GamepadMapper.
Review of attachment 355146 [details] [review]: I would first add librsvg and then GamepadView as they are two big changes, the first is related only to the build system and the other is a new class.
Created attachment 355400 [details] [review] data: Add standard-gamepad.svg This image will be used to guide the users through gamepad mapping.
Created attachment 355401 [details] [review] build: Add librsvg-2.0 This can be used to highlight individual elements of an svg image.
Created attachment 355402 [details] [review] gamepad: Add GamepadInput vapi This will be used for building the gamepad mappings.
Created attachment 355403 [details] [review] gamepad: Add standard-gamepad-inputs This will be used for providing standard gamepad inputs for building gamepad mappings.
Created attachment 355404 [details] [review] ui: Add GamepadView This adds the ability to highlight gamepad svg's individual inputs.
Created attachment 355405 [details] [review] gamepad: Add GamepadDpad vapi This will help us save the previous state of DPads for building the gamepad mappings.
Created attachment 355406 [details] [review] gamepad: Add GamepadMappingBuilder This will be used to build the sdl string from user's inputs.
Created attachment 355407 [details] [review] gamepad: Use file uris in GamesMappingsManager This will simplify the code and increase reusability for the subsequent commits.
Created attachment 355408 [details] [review] gamepad: Add GamepadMapping vapi This will be used to add set_mapping() in Gamepad.
Created attachment 355409 [details] [review] gamepad: Add set_mapping() to Gamepad This will allow us to change mappings of Gamepad even after its construction.
Created attachment 355410 [details] [review] gamepad: Free Gamepad of GamepadMappingsManager The mapping for Gamepad should be set externally. Gamepad should not have knowledge about GamepadMappingsManager.
Created attachment 355411 [details] [review] gamepad: Separate mappings in GamepadMappingsManager This will allow querying the default mappings or the user mappings for a gamepad separately.
Created attachment 355412 [details] [review] gamepad: Allow MappingsManager to save user mappings This commit adds functionality in GamepadMappingsManager to allow it to save user mappings in user's config directory.
Created attachment 355413 [details] [review] gamepad: Add GamepadMappingsManager vapi This will be used to save the sdl_string using vala code.
Created attachment 355414 [details] [review] gamepad: Add 'name' & 'guid' properties to Gamepad These will be used to build mappings for gamepads in subsequent commits.
Created attachment 355415 [details] [review] ui: Add GamepadMapper GamepadMapper uses GamepadView to get the user input, GamepadMappingBuilder to build the sdl string from those inputs and save it using GamepadMappingsManager.
Created attachment 355416 [details] [review] gamepad: Add 'unplugged' signal to GamepadMonitor The motivation is that without this signal, the users of GamepadMonitor were forced to maintain an array for the plugged-in gamepads.
Created attachment 355417 [details] [review] ui: Allow PreferencesPage to show custom headers This will allow the PreferencesPages to show widgets in the header bar. This is based on the tentative designs at https://wiki.gnome.org/Design/Apps/Settings
Created attachment 355418 [details] [review] ui: Add preferences page for inputs This screen will allow the users to edit the settings related to inputs like gamepads and keyboard.
When a mapping is in progress and we click on some other page of the window (Video, Extensions) and then go back to the Input page, the mapping process is still ongoing, it should have been canceled instead.
Review of attachment 355400 [details] [review]: Just merge this with the commit where you add GamepadView, it's a part of the widget as much as a .ui file would be.
Review of attachment 355401 [details] [review]: LGTM.
Review of attachment 355402 [details] [review]: LGTM.
Review of attachment 355403 [details] [review]: I'm still not convinced why it's not part of the mapper as it's used exclusively by it.
Review of attachment 355404 [details] [review]: ::: src/ui/gamepad-view.vala @@ +78,3 @@ + + private Cairo.Context create_similar_context (Cairo.Context context) { + var w = get_allocated_width (); w and h should be floating point numbers. @@ +91,3 @@ + var image_ratio = handle.width / handle.height; + + if (allocation_ratio > image_ratio) { A nitpick: don't use braces for only one instruction. @@ +94,3 @@ + scale = h / handle.height; + } + else { Ditto.
Review of attachment 355405 [details] [review]: LGTM.
Created attachment 356189 [details] [review] gamepad: Add GamepadViewConfiguration This provides svg information to GamepadView so it can be used for multiple svgs.
Created attachment 356190 [details] [review] ui: Add GamepadView This adds the ability to highlight gamepad svg's individual inputs.
Created attachment 356191 [details] [review] gamepad: Add GamepadDpad vapi This will be used to save the previous state of DPads for building the gamepad mappings.
Created attachment 356192 [details] [review] gamepad: Add GamepadMappingBuilder This will be used to build the sdl string corresponding to gamepad inputs.
Created attachment 356193 [details] [review] gamepad: Use file uris in GamesMappingsManager This will simplify the code and increase reusability for the subsequent commits.
Created attachment 356194 [details] [review] gamepad: Add GamepadMapping vapi This will be used to add set_mapping() in Gamepad.
Created attachment 356195 [details] [review] gamepad: Add set_mapping() to Gamepad This will allow us to change mapping of a Gamepad even after its construction.
Created attachment 356196 [details] [review] gamepad: Free Gamepad of GamepadMappingsManager The mapping for Gamepad should be set externally. Gamepad should not have knowledge about GamepadMappingsManager.
Created attachment 356197 [details] [review] gamepad: Separate mappings in GamepadMappingsManager This will allow querying the default mappings or the user mappings for a gamepad separately.
Created attachment 356198 [details] [review] gamepad: Allow MappingsManager to save user mappings This commit adds functionality in GamepadMappingsManager to allow it to save and delete user mappings in user's config directory.
Created attachment 356199 [details] [review] gamepad: Add GamepadMappingsManager vapi This will be used to save the sdl_string through UI code.
Created attachment 356200 [details] [review] gamepad: Add 'name' & 'guid' properties to Gamepad These will be used to build mappings for gamepads in subsequent commits.
Created attachment 356201 [details] [review] gamepad: Allow incomplete mappings Ignore a gamepad input if the mapping doesn't contain it.
Created attachment 356202 [details] [review] ui: Add GamepadMapper Use GamepadView to get the user input and GamepadMappingBuilder to build the sdl string from those inputs.
Created attachment 356203 [details] [review] ui: Add GamepadTester This allows the user to test their gamepads by highlighting GamepadView widget on user inputs.
Created attachment 356204 [details] [review] ui: Add GamepadConfigurer Allows the user to create, save, and reset gamepad mappings.
Created attachment 356205 [details] [review] gamepad: Add 'unplugged' signal to GamepadMonitor The motivation is that without this signal, the users of GamepadMonitor were forced to maintain an array for the plugged-in gamepads.
Created attachment 356206 [details] [review] ui: Allow PreferencesPage to show custom headers This will allow the PreferencesPages to show widgets in the header bar. Add 'immersive-mode' for PreferencesPages that allows an enhanced experience for editing input configurations.
Created attachment 356207 [details] [review] ui: Add preferences page for inputs This screen will allow the users to edit the settings related to inputs like gamepads and keyboard.
Created attachment 356255 [details] [review] data: Add standard-gamepad.svg This image will be used to guide the users through gamepad mapping.
Created attachment 356256 [details] [review] data: Add standard-gamepad.svg This image will be used to guide the users through gamepad mapping.
Created attachment 356257 [details] [review] build: Add librsvg-2.0 This can be used to highlight individual elements of an svg image.
Created attachment 356258 [details] [review] gamepad: Add GamepadInput vapi This will be used for building gamepad mappings.
Created attachment 356259 [details] [review] gamepad: Add GamepadViewConfiguration This provides svg information to GamepadView so it can be used for multiple svgs.
Created attachment 356260 [details] [review] ui: Add GamepadView This adds the ability to highlight gamepad svg's individual inputs.
Created attachment 356261 [details] [review] gamepad: Add GamepadDpad vapi This will be used to save the previous state of DPads for building the gamepad mappings.
Created attachment 356262 [details] [review] gamepad: Add GamepadMappingBuilder This will be used to build the sdl string corresponding to gamepad inputs.
Created attachment 356263 [details] [review] gamepad: Use file uris in GamesMappingsManager This will simplify the code and increase reusability for the subsequent commits.
Created attachment 356264 [details] [review] gamepad: Add GamepadMapping vapi This will be used to add set_mapping() in Gamepad.
Created attachment 356265 [details] [review] gamepad: Add set_mapping() to Gamepad This will allow us to change mapping of a Gamepad even after its construction.
Created attachment 356266 [details] [review] gamepad: Free Gamepad of GamepadMappingsManager The mapping for Gamepad should be set externally. Gamepad should not have knowledge about GamepadMappingsManager.
Created attachment 356267 [details] [review] gamepad: Separate mappings in GamepadMappingsManager This will allow querying the default mappings or the user mappings for a gamepad separately.
Created attachment 356268 [details] [review] gamepad: Allow MappingsManager to save user mappings This commit adds functionality in GamepadMappingsManager to allow it to save and delete user mappings in user's config directory.
Created attachment 356269 [details] [review] gamepad: Add GamepadMappingsManager vapi This will be used to save the sdl_string through UI code.
Created attachment 356270 [details] [review] gamepad: Add 'name' & 'guid' properties to Gamepad These will be used to build mappings for gamepads in subsequent commits.
Created attachment 356271 [details] [review] gamepad: Allow incomplete mappings Ignore a gamepad input if the mapping doesn't contain it.
Created attachment 356272 [details] [review] ui: Add GamepadMapper Use GamepadView to get the user input and GamepadMappingBuilder to build the sdl string from those inputs.
Created attachment 356273 [details] [review] ui: Add GamepadTester This allows the user to test their gamepads by highlighting GamepadView widget on user inputs.
Created attachment 356274 [details] [review] ui: Add GamepadConfigurer Allows the user to create, save, and reset gamepad mappings.
Created attachment 356275 [details] [review] gamepad: Add 'unplugged' signal to GamepadMonitor The motivation is that without this signal, the users of GamepadMonitor were forced to maintain an array for the plugged-in gamepads.
Created attachment 356276 [details] [review] ui: Allow PreferencesPage to show custom headers This will allow the PreferencesPages to show widgets in the header bar. Add 'immersive-mode' for PreferencesPages that allows an enhanced experience for editing input configurations.
Created attachment 356277 [details] [review] ui: Add preferences page for inputs This screen will allow the users to edit the settings related to inputs like gamepads and keyboard.
Review of attachment 356256 [details] [review]: LGTM.
Review of attachment 356257 [details] [review]: I would rather add this just before using the new library, but it can be reordered later.
Review of attachment 356258 [details] [review]: LGTM.
Review of attachment 356259 [details] [review]: LGTM.
Review of attachment 356260 [details] [review]: I see you are catching an error in the constructor which let the widget unusable without warning the calling code as the SVG handle would be invalid. I also notice that in later commits you built the view directly in the code thather than in the UI descriptors. What do you think about instead: - removing the constructor so you can add the view in the UI files, - have by default a usable empty state state (just drawing nothing is fine), - add a setter function doing what the constructor does now (except it could throw errors).
Review of attachment 356261 [details] [review]: LGTM.
Review of attachment 356262 [details] [review]: Overall it looks great! I have a few nitpicks unfortunately. ::: src/gamepad/gamepad-mapping-builder.vala @@ +7,3 @@ + } + + private const GamepadInputSource[] input_sources = { Constants are typically in UPPER_SNAKE_CASE. @@ +45,3 @@ + + public string build_sdl_string () { + var sdl_string = "platform:Linux" + ","; Why not "platform:Linux,"? @@ +48,3 @@ + + foreach (var mapping in mappings) + sdl_string += mapping.source_string + ":" + mapping.destination_string + ","; What about @"$(mapping.source_string):$(mapping.destination_string),"? @@ +54,3 @@ + + public bool set_button_mapping (uint8 hardware_index, GamepadInput source) { + var destination_string = "b" + hardware_index.to_string (); Here too I would recommend using string templates. Optionally you could use the template directly as the parameter of the next function call. @@ +60,3 @@ + + public bool set_axis_mapping (uint8 hardware_index, GamepadInput source) { + var destination_string = "a" + hardware_index.to_string (); Ditto. @@ +69,3 @@ + var dpad_axis = hardware_index % 2; + + var destination_string = "h" + dpad_index.to_string (); Here too I would recommend using string templates. @@ +70,3 @@ + + var destination_string = "h" + dpad_index.to_string (); + destination_string += "."; Why not adding this with the previous line? @@ +78,3 @@ + dpad.axis_values[dpad_axis] = value; + + // add 4 so the remainder is positive Nitpick: we use capitals and full punctuation for comments. @@ +92,3 @@ + var source_string = get_mapping_source (source); + if (source_string == null) { + warning ("Invalid input"); Warning messages are mostly used to warn of the bad usage of a public API. If you want to have a trace of an error: either it is expectable and normal, like here, and you use should use debug(), or it isn't expectable and we can't do a thing to fix it and you warn the user with critical(). To sum things up and from what I understand: - debug(): simmply trace a regular exception. - warning(): warn the user of our public API that they use it wrongly. - critical(): something weird happened, may not work as expected from now on. @@ +96,3 @@ + return false; + } + GamepadInputMapping mapping = {source_string, destination_string}; Nitpick: we typically leave a space { inside, curly, brackets, too }.
Review of attachment 356263 [details] [review]: Just some questions, it overall looks good. ::: src/gamepad/gamepad-mappings-manager.c @@ -73,3 @@ &inner_error); if (G_UNLIKELY (inner_error != NULL)) { - g_assert (mapping_string == NULL); Why did you remove this? @@ +82,2 @@ add_mapping (self, mapping_string); + g_free (mapping_string); Good catch, it would be better to add it as its own fix commit though so we don't loose the information of the fix. @@ +125,3 @@ GamesGamepadMappingsManager *self = NULL; gchar *dir; + const gchar *default_mappings_uri = "resource:///org/gnome/Games/gamepads/gamecontrollerdb.txt"; Private constants typically use UPPER_SNAKE_CASE and are declared at the top of the .c file, just before the private section. @@ +127,3 @@ + const gchar *default_mappings_uri = "resource:///org/gnome/Games/gamepads/gamecontrollerdb.txt"; + gchar *user_mappings_uri; + const gchar *error_message = "GamepadMappingsManager: Can’t add mappings from %s: %s"; It's nice to think about this, but it would be better to write it two times, for readability of what we send when we send it.
Review of attachment 356264 [details] [review]: LGTM.
Review of attachment 356265 [details] [review]: LGTM.
Review of attachment 356266 [details] [review]: LGTM, but fixing the two nitpicks would be nice. ::: src/gamepad/gamepad.c @@ +235,2 @@ GamesGamepad * +games_gamepad_new (GamesRawGamepad *raw_gamepad) No need for two spaces in GamesRawGamepad *raw_gamepad. ::: src/gamepad/gamepad.h @@ +14,3 @@ G_DECLARE_FINAL_TYPE (GamesGamepad, games_gamepad, GAMES, GAMEPAD, GObject) +GamesGamepad *games_gamepad_new (GamesRawGamepad *raw_gamepad); No need for two spaces in GamesRawGamepad *raw_gamepad.
Review of attachment 356267 [details] [review]: LGTM.
Review of attachment 356263 [details] [review]: ::: src/gamepad/gamepad-mappings-manager.c @@ +147,3 @@ // application. dir = games_application_get_config_dir (); + user_mappings_uri = g_strconcat ("file://", Some chars may need to be escaped, I would recommend to instead: - first build the path as it was done previously, - then build the URI from the path https://developer.gnome.org/glib/stable/glib-URI-Functions.html#g-filename-to-uri
Review of attachment 356268 [details] [review]: ::: src/gamepad/gamepad-mappings-manager.c @@ +127,3 @@ +static void +save_user_mappings (GamesGamepadMappingsManager *self, + GError **error) There should be one more space to separate the types and the names in each line here. @@ +129,3 @@ + GError **error) +{ + g_return_if_fail (self != NULL); Move that after the variable declarations as they should be at the beginning of C functions. @@ +155,3 @@ + g_hash_table_iter_init (&iter, self->user_mappings); + while (g_hash_table_iter_next (&iter, &key, &value)) + { The brace should go at the end of the previous line. @@ +160,3 @@ + sdl_string = (gchar *) value; + + mapping_string = g_strconcat (guid, ",", name, ",", sdl_string, "\n", NULL); I am wondering what is better between what you did and: g_strdup_printf ("%s,%s,%s\n", guid, name, sdl_string); @@ +209,3 @@ // application. dir = games_application_get_config_dir (); + self->user_mappings_uri = g_strconcat ("file://", Indentation is broken. @@ +297,3 @@ + save_user_mappings (self, &inner_error); + if (G_UNLIKELY (inner_error != NULL)) { + g_warning ("GamepadMappingsManager: Can’t save user mappings: %s", inner_error->message); g_critical would be better as it's an important error but we can't do a thing about it. ::: src/gamepad/gamepad-mappings-manager.h @@ +25,1 @@ G_END_DECLS It would be better to let the empty line before G_END_DECLS.
Review of attachment 356269 [details] [review]: LGTM.
Review of attachment 356270 [details] [review]: Adding that first to RawGamepad and then to Gamepad would be better, but given the patch stays small and simple it's OK.
Review of attachment 356271 [details] [review]: This doesn't match the coding style of Games: you imbricate lots of blocks when we try as much as possible to instead return early, it tends to make the code more readable and more maintainable. ::: src/gamepad/gamepad-mapping.c @@ +278,3 @@ + destination->type = EV_MAX; + if (dpad_index < self->dpads->len) { An example of how to return early: if (dpad_index >+ self->dpads->len) return;
Review of attachment 356272 [details] [review]: ::: src/ui/gamepad-mapper.vala @@ +6,3 @@ + + private GamepadInput? _input; + private GamepadInput? input { Using a property look a bit overcomplicated when you set the variable at only one place, hence you run the label update code at only one place too. What about having a function to update the label and storing the index in the array instead?
Review of attachment 356273 [details] [review]: LGTM, besides extremely small nitpicks. ::: src/ui/gamepad-tester.vala @@ +67,3 @@ + private void on_button_event (EventGamepadButton event, bool pressed) { + var highlight = pressed; + gamepad_view.highlight ({EventCode.EV_KEY, event.button}, highlight); We add spaces at the inside of curly braces too: { EventCode.EV_KEY, event.button } @@ +72,3 @@ + private void on_axis_event (EventGamepadAxis event) { + var highlight = !(-0.8 < event.gamepad_axis.value < 0.8); + gamepad_view.highlight ({EventCode.EV_ABS, event.axis}, highlight); Ditto. @@ +77,3 @@ + private void on_hat_event (EventGamepadHat event) { + var highlight = !(event.gamepad_hat.value == 0); + gamepad_view.highlight ({EventCode.EV_ABS, event.axis}, highlight); Ditto.
Review of attachment 356274 [details] [review]: ::: src/ui/gamepad-configurer.vala @@ +4,3 @@ +private class Games.GamepadConfigurer : Gtk.Box { + + private const GamepadInput[] STANDARD_GAMEPAD_INPUTS = { What about using the same order as Steam? I'll attach screenshots. @@ +28,3 @@ + }; + + private const GamepadInputPath[] STANDARD_GAMEPAD_INPUT_PATHS = { It's sorted, perfect! :) @@ +64,3 @@ + + private State? _state; + private State? state { Is it required for it to be nullable? @@ +77,3 @@ + cancel_button.hide (); + action_bar.show (); + header_bar.title = _("Testing ") + gamepad.name; This should be: /* translators: testing a gamepad, %s is its name */ header_bar.title = _("Testing %s").printf (gamepad.name); @@ +90,3 @@ + cancel_button.show (); + action_bar.hide (); + header_bar.title = _("Configuring ") + gamepad.name; Ditto. @@ +112,3 @@ + + private bool _immersive_mode; + public bool immersive_mode { Why not that instead? public bool immersive_mode { private set; get; } @@ +212,3 @@ + } + catch (Error e) { + warning (e.message); critical would be better than warning as it's an important error which is neither our fault nor the user's fault.
Created attachment 356293 [details] Steam gamepad mapping 1
Created attachment 356294 [details] Steam gamepad mapping 2
Review of attachment 356275 [details] [review]: ::: src/gamepad/gamepad-monitor.c @@ +228,3 @@ + */ + signals[SIGNAL_GAMEPAD_UNPLUGGED] = + g_signal_new ("gamepad_unplugged", Signal strings use kebab-case.
Review of attachment 356277 [details] [review]: What about using the term Controller rather than Input? Input is too vague. ::: src/ui/preferences-page-inputs.vala @@ +4,3 @@ +private class Games.PreferencesPageInputs: Gtk.Stack, PreferencesPage { + private Gtk.HeaderBar _header_bar; + public Gtk.HeaderBar header_bar { Why not that instead? public Gtk.HeaderBar header_bar { protected set; get; } @@ +10,3 @@ + + private bool _immersive_mode; + public bool immersive_mode { Ditto.
Review of attachment 356276 [details] [review]: ::: src/ui/preferences-window.vala @@ +12,3 @@ + private Gtk.Box sidebar_vbox; + [GtkChild] + private Gtk.Separator separator1; You can remove the 1 from here and the UI files if you want. @@ +24,3 @@ + titlebar_box.pack_end (value); + + _right_header_bar = value; You don't update the visibility of the close button depending on immersive_mode. I would suggest to have an update() method and to call it after setting the header bar. @@ +32,3 @@ + public bool immersive_mode { + set { + header_separator.visible = !value; I would suggest to have an update() method containing all of this and to call it after setting the mode. @@ +66,3 @@ + BindingFlags.SYNC_CREATE); + immersive_mode_binding = page.bind_property ("immersive-mode", this, "immersive-mode", + BindingFlags.SYNC_CREATE); Indentation is broken here.