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

Code and API cleanup #6569

Closed
slouken opened this issue Nov 22, 2022 · 19 comments
Closed

Code and API cleanup #6569

slouken opened this issue Nov 22, 2022 · 19 comments
Milestone

Comments

@slouken
Copy link
Collaborator

slouken commented Nov 22, 2022

Review ideas in #3519 for coding style and API conventions

Discussion?

@libsdl-org/a-team

@slouken slouken added this to the 3.2.0 milestone Nov 22, 2022
@icculus
Copy link
Collaborator

icculus commented Nov 22, 2022

There are definitely places we got things wrong (SDL_Render's function naming is pretty inconsistent, for example), but I don't think we need dogma about making all API calls match a specific pattern (SDL_SubsystemVerbNoun or whatever).

I'm pretty happy with the current coding style (One True Brace, 4 spaces no tabs, etc). In fact, I started adopting it on other personal projects after decades of using a style that is very different. :)

@markand
Copy link
Contributor

markand commented Nov 22, 2022

Having a rigid convention helps in auto completion. For example if we stick to SDL_CategoryXYZ then you can start typing SDL_Render<auto-complete> and you get a list of available functions specific to rendering.

However we can also have other styles for specific common requests (like is, open, new, create, destroy). Examples

  • SDL_IsSomething,
  • SDL_(Close|Destroy)Something,
  • SDL_(Open|Create)Something,

With that we can almost immediately identify which function should be called if one want to initialize a module and use it.

SDL_Renderer *rdr;

rdr = SDL_CreateRenderer();
SDL_RendererDrawRect(rdr, ...);
SDL_DestroyRenderer(rdr)
SDL_Joystick *js;

if (SDL_IsJoystick(1)) {
    js = SDL_OpenJoystick(1);
    SDL_JoystickGetAxis(js);
    SDL_DestroyJoystick(js);
}

@1bsyl
Copy link
Contributor

1bsyl commented Nov 22, 2022

we could have both with some kind of aliased

@slouken
Copy link
Collaborator Author

slouken commented Dec 14, 2022

I kind of like this approach, suggested by @furious-programming:

I'm personally in favor of more consistency but I don't want a dogmatic requirement that everything get named SDL_SubsystemNounVerb.

I am not talking about a dogmatic requirement, but about a convention that is clear, easy to understand and compatible with IDE tools (code completion, searching text etc.) — it's just a reasonable approach. Eskil Steenberg said something about this in the video entitled. "How I program C" — it is a really good naming that makes working with code much much easier.

The only difference I use from Eskil is a different approach to getters and setters. Eskil uses get and set at the end of the identifiers, while I use them after the subsystem name. This makes it easy to distinguish regular functions from getters and setters. For example, a regular function would be named SDL_WindowMaximize, a getter would be named SDL_WindowGetTitle, and a setter would be named SDL_WindowSetTitle. More examples from the Window API:

Regular functions:

  • SDL_WindowRaise
  • SDL_WindowMinimize
  • SDL_WindowMaximize
  • SDL_WindowCreate
  • SDL_WindowCreateFrom
  • SDL_WindowUpdateSurface
  • SDL_WindowUpdateSurfaceRects ...

Getters (same pattern for setters):

  • SDL_WindowGetSize
  • SDL_WindowGetSizeMinimum
  • SDL_WindowGetSizeMaximum
  • SDL_WindowGetSizeBorders
  • SDL_WindowGetGrab
  • SDL_WindowGetGrabMouse
  • SDL_WindowGetGrabKeyboard ...

See how the words in the identifiers stack up — the beginning fragments are consistent, which is easy to filter, while getters and setters are easy to distinguish from regular functions.

Three variants of getters should also be taken into account, i.e. the prefixes Get, Has and Is, because the latter two are also used in function names currently (and should stay that way) — e.g. SDL_WindowGetTitle, SDL_JoystickIsVirtual, SDL_GameControllerHasAxis.

@furious-programming
Copy link

Below is an example from the code of my current project, showing how fabulously easy it is to guess identifiers and how function names work with code completion window and its filtering:

naming-style

The convention is more or less the same as the current SDL, except that I use the Game_ prefix to distinguish my own functions from those of SDL. The language is Free Pascal (Lazarus IDE) just in case, but that doesn't really matter — it's just a demonstration.

@1bsyl
Copy link
Contributor

1bsyl commented Dec 16, 2022

I think it should be quite easy to turn all SDL api, into SDL_SubsystemNounVerb.
gendynapi.py extract lots of information that can be processed for a big renaming. can even do sdl2-compat I think.

@1bsyl
Copy link
Contributor

1bsyl commented Dec 16, 2022

eg, using category:
"Surface", "Thread", "Render", "Window", "Audio", "Joystick", "Thread"

SDL_GetNumAudioDrivers => SDL_AudioGetNumDrivers
SDL_GetAudioDriver => SDL_AudioGetDriver
SDL_GetCurrentAudioDriver => SDL_AudioGetCurrentDriver
SDL_OpenAudio => SDL_AudioOpen
SDL_GetNumAudioDevices => SDL_AudioDeviceGetNum
SDL_GetAudioDeviceName => SDL_AudioDeviceGetName
SDL_GetAudioDeviceSpec => SDL_AudioDeviceGetSpec
SDL_GetDefaultAudioInfo => SDL_AudioGetDefaultInfo
SDL_OpenAudioDevice => SDL_AudioDeviceOpen
SDL_GetAudioStatus => SDL_AudioGetStatus
SDL_GetAudioDeviceStatus => SDL_AudioDeviceGetStatus
SDL_PauseAudio => SDL_AudioPause
SDL_PauseAudioDevice => SDL_AudioDevicePause
SDL_BuildAudioCVT => SDL_AudioBuildCVT
SDL_ConvertAudio => SDL_AudioConvert
SDL_NewAudioStream => SDL_AudioNewStream
SDL_FreeAudioStream => SDL_AudioFreeStream
SDL_MixAudio => SDL_AudioMix
SDL_MixAudioFormat => SDL_AudioMixFormat
SDL_QueueAudio => SDL_AudioQueue
SDL_DequeueAudio => SDL_AudioDequeue
SDL_GetQueuedAudioSize => SDL_AudioGetQueuedSize
SDL_ClearQueuedAudio => SDL_AudioClearQueued
SDL_LockAudio => SDL_AudioLock
SDL_LockAudioDevice => SDL_AudioDeviceLock
SDL_UnlockAudio => SDL_AudioUnlock
SDL_UnlockAudioDevice => SDL_AudioDeviceUnlock
SDL_CloseAudio => SDL_AudioClose
SDL_CloseAudioDevice => SDL_AudioDeviceClose
SDL_CreateShapedWindow => SDL_WindowCreateShaped
SDL_IsShapedWindow => SDL_WindowIsShaped
SDL_SetWindowShape => SDL_WindowSetShape
SDL_GetShapedWindowMode => SDL_WindowGetShapedMode
SDL_LockJoysticks => SDL_JoystickLock
SDL_UnlockJoysticks => SDL_JoystickUnlock
SDL_NumJoysticks => SDL_JoystickNum
SDL_GetJoystickGUIDInfo => SDL_JoystickGetGUIDInfo
SDL_CreateSurface => SDL_SurfaceCreate
SDL_CreateSurfaceFrom => SDL_SurfaceCreateFrom
SDL_FreeSurface => SDL_SurfaceFree
SDL_SetSurfacePalette => SDL_SurfaceSetPalette
SDL_LockSurface => SDL_SurfaceLock
SDL_UnlockSurface => SDL_SurfaceUnlock
SDL_SetSurfaceRLE => SDL_SurfaceSetRLE
SDL_HasSurfaceRLE => SDL_SurfaceHasRLE
SDL_SetSurfaceColorMod => SDL_SurfaceSetColorMod
SDL_GetSurfaceColorMod => SDL_SurfaceGetColorMod
SDL_SetSurfaceAlphaMod => SDL_SurfaceSetAlphaMod
SDL_GetSurfaceAlphaMod => SDL_SurfaceGetAlphaMod
SDL_SetSurfaceBlendMode => SDL_SurfaceSetBlendMode
SDL_GetSurfaceBlendMode => SDL_SurfaceGetBlendMode
SDL_DuplicateSurface => SDL_SurfaceDuplicate
SDL_ConvertSurface => SDL_SurfaceConvert
SDL_ConvertSurfaceFormat => SDL_SurfaceConvertFormat
SDL_Vulkan_CreateSurface => SDL_SurfaceVulkan_Create
SDL_CreateThread => SDL_ThreadCreate
SDL_CreateThreadWithStackSize => SDL_ThreadCreateWithStackSize
SDL_CreateThread => SDL_ThreadCreate
SDL_CreateThreadWithStackSize => SDL_ThreadCreateWithStackSize
SDL_GetThreadName => SDL_ThreadGetName
SDL_GetThreadID => SDL_ThreadGetID
SDL_SetThreadPriority => SDL_ThreadSetPriority
SDL_WaitThread => SDL_ThreadWait
SDL_DetachThread => SDL_ThreadDetach
SDL_WarpMouseInWindow => SDL_WindowWarpMouseIn
SDL_GetWindowWMInfo => SDL_WindowGetWMInfo
SDL_GetWindowDisplayIndex => SDL_WindowGetDisplayIndex
SDL_SetWindowDisplayMode => SDL_WindowSetDisplayMode
SDL_GetWindowDisplayMode => SDL_WindowGetDisplayMode
SDL_GetWindowICCProfile => SDL_WindowGetICCProfile
SDL_GetWindowPixelFormat => SDL_WindowGetPixelFormat
SDL_CreateWindow => SDL_WindowCreate
SDL_CreateWindowFrom => SDL_WindowCreateFrom
SDL_GetWindowID => SDL_WindowGetID
SDL_GetWindowFromID => SDL_WindowGetFromID
SDL_GetWindowFlags => SDL_WindowGetFlags
SDL_SetWindowTitle => SDL_WindowSetTitle
SDL_GetWindowTitle => SDL_WindowGetTitle
SDL_SetWindowIcon => SDL_WindowSetIcon
SDL_SetWindowData => SDL_WindowSetData
SDL_GetWindowData => SDL_WindowGetData
SDL_SetWindowPosition => SDL_WindowSetPosition
SDL_GetWindowPosition => SDL_WindowGetPosition
SDL_SetWindowSize => SDL_WindowSetSize
SDL_GetWindowSize => SDL_WindowGetSize
SDL_GetWindowBordersSize => SDL_WindowGetBordersSize
SDL_GetWindowSizeInPixels => SDL_WindowGetSizeInPixels
SDL_SetWindowMinimumSize => SDL_WindowSetMinimumSize
SDL_GetWindowMinimumSize => SDL_WindowGetMinimumSize
SDL_SetWindowMaximumSize => SDL_WindowSetMaximumSize
SDL_GetWindowMaximumSize => SDL_WindowGetMaximumSize
SDL_SetWindowBordered => SDL_WindowSetBordered
SDL_SetWindowResizable => SDL_WindowSetResizable
SDL_SetWindowAlwaysOnTop => SDL_WindowSetAlwaysOnTop
SDL_ShowWindow => SDL_WindowShow
SDL_HideWindow => SDL_WindowHide
SDL_RaiseWindow => SDL_WindowRaise
SDL_MaximizeWindow => SDL_WindowMaximize
SDL_MinimizeWindow => SDL_WindowMinimize
SDL_RestoreWindow => SDL_WindowRestore
SDL_SetWindowFullscreen => SDL_WindowSetFullscreen
SDL_GetWindowSurface => SDL_SurfaceGetWindow
SDL_UpdateWindowSurface => SDL_SurfaceUpdateWindow
SDL_UpdateWindowSurfaceRects => SDL_SurfaceUpdateWindowRects
SDL_SetWindowGrab => SDL_WindowSetGrab
SDL_SetWindowKeyboardGrab => SDL_WindowSetKeyboardGrab
SDL_SetWindowMouseGrab => SDL_WindowSetMouseGrab
SDL_GetWindowGrab => SDL_WindowGetGrab
SDL_GetWindowKeyboardGrab => SDL_WindowGetKeyboardGrab
SDL_GetWindowMouseGrab => SDL_WindowGetMouseGrab
SDL_GetGrabbedWindow => SDL_WindowGetGrabbed
SDL_SetWindowMouseRect => SDL_WindowSetMouseRect
SDL_GetWindowMouseRect => SDL_WindowGetMouseRect
SDL_SetWindowOpacity => SDL_WindowSetOpacity
SDL_GetWindowOpacity => SDL_WindowGetOpacity
SDL_SetWindowModalFor => SDL_WindowSetModalFor
SDL_SetWindowInputFocus => SDL_WindowSetInputFocus
SDL_SetWindowHitTest => SDL_WindowSetHitTest
SDL_FlashWindow => SDL_WindowFlash
SDL_DestroyWindow => SDL_WindowDestroy
SDL_GL_GetCurrentWindow => SDL_WindowGL_GetCurrent
SDL_EGL_GetWindowEGLSurface => SDL_SurfaceEGL_GetWindowEGL
SDL_GL_SwapWindow => SDL_WindowGL_Swap
SDL_GameControllerGetJoystick => SDL_JoystickGameControllerGet
SDL_HapticOpenFromJoystick => SDL_JoystickHapticOpenFrom
SDL_GetNumRenderDrivers => SDL_RenderGetNumDrivers
SDL_GetRenderDriver => SDL_RenderGetDriver
SDL_CreateWindowAndRenderer => SDL_RendererCreateWindowAnd
SDL_CreateRenderer => SDL_RendererCreate
SDL_CreateSoftwareRenderer => SDL_RendererCreateSoftware
SDL_GetRenderer => SDL_RendererGet
SDL_GetRendererInfo => SDL_RendererGetInfo
SDL_GetRendererOutputSize => SDL_RendererGetOutputSize
SDL_CreateTextureFromSurface => SDL_SurfaceCreateTextureFrom
SDL_LockTextureToSurface => SDL_SurfaceLockTextureTo
SDL_SetRenderTarget => SDL_RenderSetTarget
SDL_GetRenderTarget => SDL_RenderGetTarget
SDL_SetRenderDrawColor => SDL_RenderSetDrawColor
SDL_GetRenderDrawColor => SDL_RenderGetDrawColor
SDL_SetRenderDrawBlendMode => SDL_RenderSetDrawBlendMode
SDL_GetRenderDrawBlendMode => SDL_RenderGetDrawBlendMode
SDL_DestroyRenderer => SDL_RendererDestroy
SDL_SetWindowsMessageHook => SDL_WindowSetMessageHook
SDL_LinuxSetThreadPriority => SDL_ThreadLinuxSetPriority
SDL_LinuxSetThreadPriorityAndPolicy => SDL_ThreadLinuxSetPriorityAndPolicy

@1bsyl
Copy link
Contributor

1bsyl commented Dec 16, 2022

(with a bug "render vs renderer" inside)

@1bsyl
Copy link
Contributor

1bsyl commented Dec 16, 2022

while it makes search easy, it makes code less readable

@furious-programming
Copy link

furious-programming commented Dec 16, 2022

@1bsyl: why ”less readable”? It is the use of a specific pattern that allows you to increase readability, because the construction of identifiers is uniform and always predictable. Source code is not a poem — identifiers should be constructed so that you don't have to memorize them, instead of following the rules of English sentence formation.

Your proposal (a list of new function names) is not good because, for example, you can't filter out audio device functions because you don't use the AudioDevice module prefix — they are mixed with all audio functions. It starts with the Audio prefix, but the Device is in completely different places:

SDL_AudioGetStatus
SDL_AudioOpenDevice
SDL_AudioGetDeviceStatus
SDL_AudioConvert

Different languages and for different purposes use a prefix system in which the common parts of identifiers are at the beginning of them. Unfortunately, such a system is mainly used for naming constants, while there is chaos in the names of functions. Meanwhile, nothing stands in the way of using such a naming system in the case of functions (as well as constants and data types), introducing uniformity and order in the entire library.

@1bsyl
Copy link
Contributor

1bsyl commented Dec 16, 2022

I mean "code less readable", because I think one spends more time reading code, than searching for function name. and so reading should have more importance than searching for a function. But this is just a point of view.

yes, the category isn't perfect. this is a 5 min proto. can add the AudioDevice category to try. l'll update the list.

@1bsyl
Copy link
Contributor

1bsyl commented Dec 16, 2022

updated

@slouken
Copy link
Collaborator Author

slouken commented Dec 23, 2022

Okay, so after discussion with @icculus, I think we're going to try with the following conventions:

  • SDL_VerbNounClarification
  • SDL_Create* + SDL_Destroy*

I'm going to start with SDL_audio.h as a first example for discussion and we can move on from there.

@slouken
Copy link
Collaborator Author

slouken commented Dec 23, 2022

Okay, the complete change proposal is up, and #6876 has been updated with the list of headers that aren't touched.

One nice thing is that with this new proposal, only 9 out of 79 API headers are being touched, which means that this is largely in line with how we've been designing the API.

Comments welcome!

@apartfromtime
Copy link

apartfromtime commented Dec 23, 2022

I write my code <api>_<function|procedure><domain|namespace><identifiers> which is inline with <api>_<verb><noun><clarification>. I try not to think of domains or namespaces as objects to me an object is for instance a "window handle" in a stack of windows. In the cases of ambiguity restructure\rename any code to remove the the ambiguity.

@slouken
Copy link
Collaborator Author

slouken commented Dec 27, 2022

The bulk of this work is done!

Feel free to open new issues for additional cleanup or to revert bad ideas. :)

@slouken slouken closed this as completed Dec 27, 2022
@AntTheAlchemist
Copy link
Contributor

I've just been going through the SDL2 to SDL3 migration and noticed some possible niggles:

Should we rename functions like SDL_GetGamepadNumTouchpadFingers to SDL_GetNumGamepadTouchFingers to match other conventions? We now have SDL_GetNumJoystickButtons for example.

And I noticed missing oldtype #define for the following:

SDL_GameControllerTypeForIndex
SDL_GameControllerNameForIndex
SDL_JoystickNameForIndex
SDL_NumJoysticks
SDL_SensorGetDeviceType
SDL_SensorGetDeviceName

@slouken
Copy link
Collaborator Author

slouken commented Jan 24, 2023

Should we rename functions like SDL_GetGamepadNumTouchpadFingers to SDL_GetNumGamepadTouchFingers to match other conventions?

Yep, this is done in a06a593.

@slouken
Copy link
Collaborator Author

slouken commented Jan 24, 2023

And I noticed missing oldtype #define for the following:

SDL_GameControllerTypeForIndex
SDL_GameControllerNameForIndex
SDL_JoystickNameForIndex
SDL_NumJoysticks
SDL_SensorGetDeviceType
SDL_SensorGetDeviceName

There are no direct equivalents for these functions. Instead you get a list of device IDs and operate on those with new functions.

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

No branches or pull requests

7 participants