1# Contributing to wlroots 2 3Contributing just involves sending a pull request. You will probably be more 4successful with your contribution if you visit 5[#sway-devel](https://webchat.freenode.net/?channels=sway-devel) on 6irc.freenode.net upfront and discuss your plans. 7 8Note: rules are made to be broken. Adjust or ignore any/all of these as you see 9fit, but be prepared to justify it to your peers. 10 11## Pull Requests 12 13If you already have your own pull request habits, feel free to use them. If you 14don't, however, allow me to make a suggestion: feature branches pulled from 15upstream. Try this: 16 171. Fork wlroots 182. `git clone https://github.com/username/wlroots && cd wlroots` 193. `git remote add upstream https://github.com/swaywm/wlroots` 20 21You only need to do this once. You're never going to use your fork's master 22branch. Instead, when you start working on a feature, do this: 23 241. `git fetch upstream` 252. `git checkout -b add-so-and-so-feature upstream/master` 263. Add and commit your changes 274. `git push -u origin add-so-and-so-feature` 285. Make a pull request from your feature branch 29 30When you submit your pull request, your commit log should do most of the talking 31when it comes to describing your changes and their motivation. In addition to 32this, your pull request's comments will ideally include a test plan that the 33reviewers can use to (1) demonstrate the problem on master, if applicable and 34(2) verify that the problem no longer exists with your changes applied (or that 35your new features work correctly). Document all of the edge cases you're aware 36of so we can adequately test them - then verify the test plan yourself before 37submitting. 38 39## Commit Messages 40 41Please strive to write good commit messages. Here's some guidelines to follow: 42 43The first line should be limited to 50 characters and should be a sentence that 44completes the thought [When applied, this commit will...] *"Implement 45cmd_move"* or *"Fix #742"* or *"Improve performance of arrange_windows on ARM"* 46or similar. 47 48The subsequent lines should be separated from the subject line by a single 49blank line, and include optional details. In this you can give justification 50for the change, [reference Github 51issues](https://help.github.com/articles/closing-issues-via-commit-messages/), 52or explain some of the subtler details of your patch. This is important because 53when someone finds a line of code they don't understand later, they can use the 54`git blame` command to find out what the author was thinking when they wrote 55it. It's also easier to review your pull requests if they're separated into 56logical commits that have good commit messages and justify themselves in the 57extended commit description. 58 59As a good rule of thumb, anything you might put into the pull request 60description on Github is probably fair game for going into the extended commit 61message as well. 62 63See [here](https://chris.beams.io/posts/git-commit/) for more details. 64 65## Code Review 66 67When your changes are submitted for review, one or more core committers will 68look over them. Smaller changes might be merged with little fanfare, but larger 69changes will typically see review from several people. Be prepared to receive 70some feedback - you may be asked to make changes to your work. Our code review 71process is: 72 731. **Triage** the pull request. Do the commit messages make sense? Is a test 74 plan necessary and/or present? Add anyone as reviewers that you think should 75 be there (using the relevant GitHub feature, if you have the permissions, or 76 with an @mention if necessary). 772. **Review** the code. Look for code style violations, naming convention 78 violations, buffer overflows, memory leaks, logic errors, non-portable code 79 (including GNU-isms), etc. For significant changes to the public API, loop in 80 a couple more people for discussion. 813. **Execute** the test plan, if present. 824. **Merge** the pull request when all reviewers approve. 835. **File** follow-up tickets if appropriate. 84 85## Style Reference 86 87wlroots is written in C with a style similar to the [kernel 88style](https://www.kernel.org/doc/Documentation/process/coding-style.rst), but 89with a few notable differences. 90 91Try to keep your code conforming to C11 and POSIX as much as possible, and do 92not use GNU extensions. 93 94### Brackets 95 96Brackets always go on the same line, including in functions. 97Always include brackets for if/while/for, even if it's a single statement. 98```c 99void function(void) { 100 if (condition1) { 101 do_thing1(); 102 } 103 104 if (condition2) { 105 do_thing2(); 106 } else { 107 do_thing3(); 108 } 109} 110``` 111 112### Indentation 113 114Indentations are a single tab. 115 116For long lines that need to be broken, the continuation line should be indented 117with an additional tab. 118If the line being broken is opening a new block (functions, if, while, etc.), 119the continuation line should be indented with two tabs, so they can't be 120misread as being part of the block. 121```c 122really_long_function(argument1, argument2, ..., 123 argument3, argument4); 124 125if (condition1 && condition2 && ... 126 condition3 && condition4) { 127 do_thing(); 128} 129``` 130 131Try to break the line in the place which you think is the most appropriate. 132 133 134### Line Length 135 136Try to keep your lines under 80 columns, but you can go up to 100 if it 137improves readability. Don't break lines indiscriminately, try to find nice 138breaking points so your code is easy to read. 139 140### Names 141 142Global function and type names should be prefixed with `wlr_submodule_` (e.g. 143`struct wlr_output`, `wlr_output_set_cursor`). For static functions and 144types local to a file, the names chosen aren't as important. Local function 145names shouldn't have a `wlr_` prefix. 146 147For include guards, use the header's filename relative to include. Uppercase 148all of the characters, and replace any invalid characters with an underscore. 149 150### Construction/Destruction Functions 151 152For functions that are responsible for constructing and destructing an object, 153they should be written as a pair of one of two forms: 154* `init`/`finish`: These initialize/deinitialize a type, but are **NOT** 155responsible for allocating it. They should accept a pointer to some 156pre-allocated memory (e.g. a member of a struct). 157* `create`/`destroy`: These also initialize/deinitialize, but will return a 158pointer to a `malloc`ed chunk of memory, and will `free` it in `destroy`. 159 160A destruction function should always be able to accept a NULL pointer or a 161zeroed value and exit cleanly; this simplifies error handling a lot. 162 163### Error Codes 164 165For functions not returning a value, they should return a (stdbool.h) bool to 166indicated if they succeeded or not. 167 168### Macros 169 170Try to keep the use of macros to a minimum, especially if a function can do the 171job. If you do need to use them, try to keep them close to where they're being 172used and `#undef` them after. 173 174### Example 175 176```c 177struct wlr_backend *wlr_backend_autocreate(struct wl_display *display) { 178 struct wlr_backend *backend; 179 if (getenv("WAYLAND_DISPLAY") || getenv("_WAYLAND_DISPLAY")) { 180 backend = attempt_wl_backend(display); 181 if (backend) { 182 return backend; 183 } 184 } 185 186 const char *x11_display = getenv("DISPLAY"); 187 if (x11_display) { 188 return wlr_x11_backend_create(display, x11_display); 189 } 190 191 // Attempt DRM+libinput 192 193 struct wlr_session *session = wlr_session_create(display); 194 if (!session) { 195 wlr_log(WLR_ERROR, "Failed to start a DRM session"); 196 return NULL; 197 } 198 199 int gpu = wlr_session_find_gpu(session); 200 if (gpu == -1) { 201 wlr_log(WLR_ERROR, "Failed to open DRM device"); 202 goto error_session; 203 } 204 205 backend = wlr_multi_backend_create(session); 206 if (!backend) { 207 goto error_gpu; 208 } 209 210 struct wlr_backend *libinput = wlr_libinput_backend_create(display, session); 211 if (!libinput) { 212 goto error_multi; 213 } 214 215 struct wlr_backend *drm = wlr_drm_backend_create(display, session, gpu); 216 if (!drm) { 217 goto error_libinput; 218 } 219 220 wlr_multi_backend_add(backend, libinput); 221 wlr_multi_backend_add(backend, drm); 222 return backend; 223 224error_libinput: 225 wlr_backend_destroy(libinput); 226error_multi: 227 wlr_backend_destroy(backend); 228error_gpu: 229 wlr_session_close_file(session, gpu); 230error_session: 231 wlr_session_destroy(session); 232 return NULL; 233} 234``` 235 236## Wayland protocol implementation 237 238Each protocol generally lives in a file with the same name, usually containing 239at least one struct for each interface in the protocol. For instance, 240`xdg_shell` lives in `types/wlr_xdg_shell.h` and has a `wlr_xdg_surface` struct. 241 242### Globals 243 244Global interfaces generally have public constructors and destructors. Their 245struct has a field holding the `wl_global` itself, a destroy signal and a 246`wl_display` destroy listener. Example: 247 248```c 249struct wlr_compositor { 250 struct wl_global *global; 251 … 252 253 struct wl_listener display_destroy; 254 255 struct { 256 struct wl_signal new_surface; 257 struct wl_signal destroy; 258 } events; 259}; 260``` 261 262When the destructor is called, it should emit the destroy signal, remove the 263display destroy listener, destroy the `wl_global` and then destroy the struct. 264The destructor can assume all clients and resources have been already 265destroyed. 266 267### Resources 268 269Resources are the representation of Wayland objects on the compositor side. They 270generally have an associated struct, called the _object struct_, stored in their 271`user_data` field. 272 273Object structs can be retrieved from resources via `wl_resource_get_data`. To 274prevent bad casts, a safe helper function checking the type of the resource is 275used: 276 277```c 278static const struct wl_surface_interface surface_impl; 279 280struct wlr_surface *wlr_surface_from_resource(struct wl_resource *resource) { 281 assert(wl_resource_instance_of(resource, &wl_surface_interface, 282 &surface_impl)); 283 return wl_resource_get_user_data(resource); 284} 285``` 286 287If a pointer to a `wl_resource` is stored, a resource destroy handler needs to 288be registered to clean it up. libwayland will automatically destroy resources 289in an arbitrary order when a client is disconnected, the compositor must handle 290this correctly. 291 292### Destroying resources 293 294Object structs should only be destroyed when their resource is destroyed, ie. 295in the resource destroy handler (set with `wl_resource_set_implementation`). 296 297- If the object has a destructor request: the request handler should just call 298 `wl_resource_destroy` and do nothing else. The compositor must not destroy 299 resources on its own outside the destructor request handler. 300- If the protocol specifies that an object is destroyed when an event is sent: 301 it's the only case where the compositor is allowed to send the event and then 302 call `wl_resource_destroy`. An example of this is `wl_callback`. 303 304### Inert resources 305 306Some resources can become inert in situations described in the protocol or when 307the compositor decides to get rid of them. All requests made to inert resources 308should be ignored, except the destructor. This is achieved by: 309 3101. When the resource becomes inert: destroy the object struct and call 311 `wl_resource_set_user_data(resource, NULL)`. Do not destroy the resource. 3122. For each request made to a resource that can be inert: add a NULL check to 313 ignore the request if the resource is inert. 3143. When the client calls the destructor request on the resource: call 315 `wl_resource_destroy(resource)` as usual. 3164. When the resource is destroyed, if the resource isn't inert, destroy the 317 object struct. 318 319Example: 320 321```c 322// Handles the destroy request 323static void subsurface_handle_destroy(struct wl_client *client, 324 struct wl_resource *resource) { 325 wl_resource_destroy(resource); 326} 327 328// Handles a regular request 329static void subsurface_set_position(struct wl_client *client, 330 struct wl_resource *resource, int32_t x, int32_t y) { 331 struct wlr_subsurface *subsurface = subsurface_from_resource(resource); 332 if (subsurface == NULL) { 333 return; 334 } 335 336 … 337} 338 339// Destroys the wlr_subsurface struct 340static void subsurface_destroy(struct wlr_subsurface *subsurface) { 341 if (subsurface == NULL) { 342 return; 343 } 344 345 … 346 347 wl_resource_set_user_data(subsurface->resource, NULL); 348 free(subsurface); 349} 350 351// Resource destroy listener 352static void subsurface_handle_resource_destroy(struct wl_resource *resource) { 353 struct wlr_subsurface *subsurface = subsurface_from_resource(resource); 354 subsurface_destroy(subsurface); 355} 356 357// Makes the resource inert 358static void subsurface_handle_surface_destroy(struct wl_listener *listener, 359 void *data) { 360 struct wlr_subsurface *subsurface = 361 wl_container_of(listener, subsurface, surface_destroy); 362 subsurface_destroy(subsurface); 363} 364``` 365