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;
251252
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
336337}
338
339// Destroys the wlr_subsurface struct
340static void subsurface_destroy(struct wlr_subsurface *subsurface) {
341	if (subsurface == NULL) {
342		return;
343	}
344
345346
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