CONTRIBUTING.md 13.9 KB
Newer Older
1
2
# Contributing to wlroots

Simon Ser's avatar
Simon Ser committed
3
Contributing just involves sending a merge request. You will probably be more
4
5
successful with your contribution if you visit [#sway-devel on Libera Chat]
upfront and discuss your plans.
6

Drew DeVault's avatar
Drew DeVault committed
7
8
9
Note: rules are made to be broken. Adjust or ignore any/all of these as you see
fit, but be prepared to justify it to your peers.

Simon Ser's avatar
Simon Ser committed
10
## Merge Requests
11

Simon Ser's avatar
Simon Ser committed
12
If you already have your own merge request habits, feel free to use them. If you
13
14
15
don't, however, allow me to make a suggestion: feature branches pulled from
upstream. Try this:

Scott Anderson's avatar
Scott Anderson committed
16
1. Fork wlroots
Simon Ser's avatar
Simon Ser committed
17
18
2. `git clone git@gitlab.freedesktop.org:<username>/wlroots.git && cd wlroots`
3. `git remote add upstream https://gitlab.freedesktop.org/wlroots/wlroots.git`
19
20
21
22

You only need to do this once. You're never going to use your fork's master
branch. Instead, when you start working on a feature, do this:

Scott Anderson's avatar
Scott Anderson committed
23
24
25
26
1. `git fetch upstream`
2. `git checkout -b add-so-and-so-feature upstream/master`
3. Add and commit your changes
4. `git push -u origin add-so-and-so-feature`
Simon Ser's avatar
Simon Ser committed
27
5. Make a merge request from your feature branch
28

Simon Ser's avatar
Simon Ser committed
29
When you submit your merge request, your commit log should do most of the talking
Drew DeVault's avatar
Drew DeVault committed
30
when it comes to describing your changes and their motivation. In addition to
Simon Ser's avatar
Simon Ser committed
31
this, your merge request's comments will ideally include a test plan that the
Drew DeVault's avatar
Drew DeVault committed
32
33
34
35
36
37
reviewers can use to (1) demonstrate the problem on master, if applicable and
(2) verify that the problem no longer exists with your changes applied (or that
your new features work correctly). Document all of the edge cases you're aware
of so we can adequately test them - then verify the test plan yourself before
submitting.

38
39
40
## Commit Log

Unlike many projects using GitHub and GitLab, wlroots has a [linear, "recipe"
41
42
43
style] history. This means that every commit should be small, digestible,
stand-alone, and functional. Rather than a purely chronological commit history
like this:
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70

```
doc: final docs for view transforms
fix tests when disabled, redo broken doc formatting
better transformed-view iteration (thanks Hannah!)
try to catch more cases in tests
tests: add new spline test
fix compilation on splines
doc: notes on reticulating splines
compositor: add spline reticulation for view transforms
```

We aim to have a clean history which only reflects the final state, broken up
into functional groupings:

```
compositor: add spline reticulation for view transforms
compositor: new iterator for view transforms
tests: add view-transform correctness tests
doc: fix formatting for view transforms
```

This ensures that the final patch series only contains the final state,
without the changes and missteps taken along the development process. A linear
history eases reviewing, cherry-picking and reverting changes.

If you aren't comfortable with manipulating the Git history, have a look at
71
[git-rebase.io].
72

73
74
75
76
77
## Commit Messages

Please strive to write good commit messages. Here's some guidelines to follow:

The first line should be limited to 50 characters and should be a sentence that
Scott Anderson's avatar
Scott Anderson committed
78
completes the thought [When applied, this commit will...] *"Implement
79
cmd_move"* or *"Improve performance of arrange_windows on ARM"* or similar.
80

Scott Anderson's avatar
Scott Anderson committed
81
The subsequent lines should be separated from the subject line by a single
82
blank line, and include optional details. In this you can give justification
Simon Ser's avatar
Simon Ser committed
83
for the change, [reference issues], or explain some of the subtler
84
85
86
details of your patch. This is important because when someone finds a line of
code they don't understand later, they can use the `git blame` command to find
out what the author was thinking when they wrote it. It's also easier to review
Simon Ser's avatar
Simon Ser committed
87
your merge requests if they're separated into logical commits that have good
88
commit messages and justify themselves in the extended commit description.
89

Simon Ser's avatar
Simon Ser committed
90
91
As a good rule of thumb, anything you might put into the merge request
description on GitLab is probably fair game for going into the extended commit
92
93
message as well.

94
See [How to Write a Git Commit Message] for more details.
Scott Anderson's avatar
Scott Anderson committed
95

Drew DeVault's avatar
Drew DeVault committed
96
97
98
99
100
101
102
103
## Code Review

When your changes are submitted for review, one or more core committers will
look over them. Smaller changes might be merged with little fanfare, but larger
changes will typically see review from several people. Be prepared to receive
some feedback - you may be asked to make changes to your work. Our code review
process is:

Simon Ser's avatar
Simon Ser committed
104
1. **Triage** the merge request. Do the commit messages make sense? Is a test
Drew DeVault's avatar
Drew DeVault committed
105
   plan necessary and/or present? Add anyone as reviewers that you think should
Simon Ser's avatar
Simon Ser committed
106
   be there (using the relevant GitLab feature, if you have the permissions, or
Drew DeVault's avatar
Drew DeVault committed
107
108
109
110
111
112
   with an @mention if necessary).
2. **Review** the code. Look for code style violations, naming convention
   violations, buffer overflows, memory leaks, logic errors, non-portable code
   (including GNU-isms), etc. For significant changes to the public API, loop in
   a couple more people for discussion.
3. **Execute** the test plan, if present.
Simon Ser's avatar
Simon Ser committed
113
4. **Merge** the merge request when all reviewers approve.
Drew DeVault's avatar
Drew DeVault committed
114
115
5. **File** follow-up tickets if appropriate.

116
117
118
119
120
121
122
## Code of Conduct

Note that as a project hosted on freedesktop.org, wlroots follows its
[Code of Conduct], based on the Contributor Covenant. Please conduct yourself
in a respectful and civilized manner when communicating with community members
on IRC and bug tracker.

Drew DeVault's avatar
Drew DeVault committed
123
## Style Reference
124

125
126
wlroots is written in C with a style similar to the [kernel style], but with a
few notable differences.
127

Scott Anderson's avatar
Scott Anderson committed
128
129
130
131
Try to keep your code conforming to C11 and POSIX as much as possible, and do
not use GNU extensions.

### Brackets
132

133
Brackets always go on the same line, including in functions.
Scott Anderson's avatar
Scott Anderson committed
134
135
Always include brackets for if/while/for, even if it's a single statement.
```c
136
void function(void) {
Scott Anderson's avatar
Scott Anderson committed
137
138
	if (condition1) {
		do_thing1();
139
	}
Scott Anderson's avatar
Scott Anderson committed
140
141
142

	if (condition2) {
		do_thing2();
143
	} else {
Scott Anderson's avatar
Scott Anderson committed
144
		do_thing3();
145
	}
Scott Anderson's avatar
Scott Anderson committed
146
147
}
```
148

Scott Anderson's avatar
Scott Anderson committed
149
### Indentation
150

Scott Anderson's avatar
Scott Anderson committed
151
Indentations are a single tab.
152

Scott Anderson's avatar
Scott Anderson committed
153
For long lines that need to be broken, the continuation line should be indented
154
with an additional tab.
Scott Anderson's avatar
Scott Anderson committed
155
156
157
158
159
160
161
162
163
164
If the line being broken is opening a new block (functions, if, while, etc.),
the continuation line should be indented with two tabs, so they can't be
misread as being part of the block.
```c
really_long_function(argument1, argument2, ...,
	argument3, argument4);

if (condition1 && condition2 && ...
		condition3 && condition4) {
	do_thing();
165
166
}
```
Scott Anderson's avatar
Scott Anderson committed
167
168
169
170
171
172
173

Try to break the line in the place which you think is the most appropriate.


### Line Length

Try to keep your lines under 80 columns, but you can go up to 100 if it
Drew DeVault's avatar
Drew DeVault committed
174
175
improves readability. Don't break lines indiscriminately, try to find nice
breaking points so your code is easy to read.
Scott Anderson's avatar
Scott Anderson committed
176
177
178

### Names

179
180
181
182
Global function and type names should be prefixed with `wlr_submodule_` (e.g.
`struct wlr_output`, `wlr_output_set_cursor`).  For static functions and
types local to a file, the names chosen aren't as important.  Local function
names shouldn't have a `wlr_` prefix.
Scott Anderson's avatar
Scott Anderson committed
183

Scott Anderson's avatar
Scott Anderson committed
184
185
186
For include guards, use the header's filename relative to include.  Uppercase
all of the characters, and replace any invalid characters with an underscore.

Scott Anderson's avatar
Scott Anderson committed
187
188
189
190
191
192
193
194
195
196
### Construction/Destruction Functions

For functions that are responsible for constructing and destructing an object,
they should be written as a pair of one of two forms:
* `init`/`finish`: These initialize/deinitialize a type, but are **NOT**
responsible for allocating it. They should accept a pointer to some
pre-allocated memory (e.g. a member of a struct).
* `create`/`destroy`: These also initialize/deinitialize, but will return a
pointer to a `malloc`ed chunk of memory, and will `free` it in `destroy`.

197
198
A destruction function should always be able to accept a NULL pointer or a
zeroed value and exit cleanly; this simplifies error handling a lot.
Scott Anderson's avatar
Scott Anderson committed
199
200
201
202

### Error Codes

For functions not returning a value, they should return a (stdbool.h) bool to
Hubert Hirtz's avatar
Hubert Hirtz committed
203
indicate whether they succeeded or not.
Scott Anderson's avatar
Scott Anderson committed
204
205
206
207
208
209
210

### Macros

Try to keep the use of macros to a minimum, especially if a function can do the
job.  If you do need to use them, try to keep them close to where they're being
used and `#undef` them after.

211
### Example
Scott Anderson's avatar
Scott Anderson committed
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228

```c
struct wlr_backend *wlr_backend_autocreate(struct wl_display *display) {
	struct wlr_backend *backend;
	if (getenv("WAYLAND_DISPLAY") || getenv("_WAYLAND_DISPLAY")) {
		backend = attempt_wl_backend(display);
		if (backend) {
			return backend;
		}
	}

	const char *x11_display = getenv("DISPLAY");
	if (x11_display) {
		return wlr_x11_backend_create(display, x11_display);
	}

	// Attempt DRM+libinput
Scott Anderson's avatar
Scott Anderson committed
229

Scott Anderson's avatar
Scott Anderson committed
230
231
	struct wlr_session *session = wlr_session_create(display);
	if (!session) {
232
		wlr_log(WLR_ERROR, "Failed to start a DRM session");
Scott Anderson's avatar
Scott Anderson committed
233
234
235
236
237
		return NULL;
	}

	int gpu = wlr_session_find_gpu(session);
	if (gpu == -1) {
238
		wlr_log(WLR_ERROR, "Failed to open DRM device");
Scott Anderson's avatar
Scott Anderson committed
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
		goto error_session;
	}

	backend = wlr_multi_backend_create(session);
	if (!backend) {
		goto error_gpu;
	}

	struct wlr_backend *libinput = wlr_libinput_backend_create(display, session);
	if (!libinput) {
		goto error_multi;
	}

	struct wlr_backend *drm = wlr_drm_backend_create(display, session, gpu);
	if (!drm) {
		goto error_libinput;
	}

	wlr_multi_backend_add(backend, libinput);
	wlr_multi_backend_add(backend, drm);
	return backend;

error_libinput:
	wlr_backend_destroy(libinput);
error_multi:
	wlr_backend_destroy(backend);
error_gpu:
	wlr_session_close_file(session, gpu);
error_session:
	wlr_session_destroy(session);
	return NULL;
}
```
272
273
274
275

## Wayland protocol implementation

Each protocol generally lives in a file with the same name, usually containing
276
at least one struct for each interface in the protocol. For instance,
277
278
279
280
281
`xdg_shell` lives in `types/wlr_xdg_shell.h` and has a `wlr_xdg_surface` struct.

### Globals

Global interfaces generally have public constructors and destructors. Their
282
283
struct has a field holding the `wl_global` itself, a destroy signal and a
`wl_display` destroy listener. Example:
284
285
286

```c
struct wlr_compositor {
287
	struct wl_global *global;
288
289
290
291
292
293
294
295
296
297
298
299
	

	struct wl_listener display_destroy;

	struct {
		struct wl_signal new_surface;
		struct wl_signal destroy;
	} events;
};
```

When the destructor is called, it should emit the destroy signal, remove the
300
301
302
display destroy listener, destroy the `wl_global` and then destroy the struct.
The destructor can assume all clients and resources have been already
destroyed.
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323

### Resources

Resources are the representation of Wayland objects on the compositor side. They
generally have an associated struct, called the _object struct_, stored in their
`user_data` field.

Object structs can be retrieved from resources via `wl_resource_get_data`. To
prevent bad casts, a safe helper function checking the type of the resource is
used:

```c
static const struct wl_surface_interface surface_impl;

struct wlr_surface *wlr_surface_from_resource(struct wl_resource *resource) {
	assert(wl_resource_instance_of(resource, &wl_surface_interface,
		&surface_impl));
	return wl_resource_get_user_data(resource);
}
```

324
325
326
327
328
If a pointer to a `wl_resource` is stored, a resource destroy handler needs to
be registered to clean it up. libwayland will automatically destroy resources
in an arbitrary order when a client is disconnected, the compositor must handle
this correctly.

329
330
331
332
333
### Destroying resources

Object structs should only be destroyed when their resource is destroyed, ie.
in the resource destroy handler (set with `wl_resource_set_implementation`).

334
335
336
337
338
339
- If the object has a destructor request: the request handler should just call
  `wl_resource_destroy` and do nothing else. The compositor must not destroy
  resources on its own outside the destructor request handler.
- If the protocol specifies that an object is destroyed when an event is sent:
  it's the only case where the compositor is allowed to send the event and then
  call `wl_resource_destroy`. An example of this is `wl_callback`.
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382

### Inert resources

Some resources can become inert in situations described in the protocol or when
the compositor decides to get rid of them. All requests made to inert resources
should be ignored, except the destructor. This is achieved by:

1. When the resource becomes inert: destroy the object struct and call
   `wl_resource_set_user_data(resource, NULL)`. Do not destroy the resource.
2. For each request made to a resource that can be inert: add a NULL check to
   ignore the request if the resource is inert.
3. When the client calls the destructor request on the resource: call
   `wl_resource_destroy(resource)` as usual.
4. When the resource is destroyed, if the resource isn't inert, destroy the
   object struct.

Example:

```c
// Handles the destroy request
static void subsurface_handle_destroy(struct wl_client *client,
		struct wl_resource *resource) {
	wl_resource_destroy(resource);
}

// Handles a regular request
static void subsurface_set_position(struct wl_client *client,
		struct wl_resource *resource, int32_t x, int32_t y) {
	struct wlr_subsurface *subsurface = subsurface_from_resource(resource);
	if (subsurface == NULL) {
		return;
	}

	
}

// Destroys the wlr_subsurface struct
static void subsurface_destroy(struct wlr_subsurface *subsurface) {
	if (subsurface == NULL) {
		return;
	}

	
383
384

	wl_resource_set_user_data(subsurface->resource, NULL);
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
	free(subsurface);
}

// Resource destroy listener
static void subsurface_handle_resource_destroy(struct wl_resource *resource) {
	struct wlr_subsurface *subsurface = subsurface_from_resource(resource);
	subsurface_destroy(subsurface);
}

// Makes the resource inert
static void subsurface_handle_surface_destroy(struct wl_listener *listener,
		void *data) {
	struct wlr_subsurface *subsurface =
		wl_container_of(listener, subsurface, surface_destroy);
	subsurface_destroy(subsurface);
}
```
402

Simon Ser's avatar
Simon Ser committed
403
[#sway-devel on Libera Chat]: https://web.libera.chat/gamja/?channels=#sway-devel
404
405
[linear, "recipe" style]: https://www.bitsnbites.eu/git-history-work-log-vs-recipe/
[git-rebase.io]: https://git-rebase.io/
Simon Ser's avatar
Simon Ser committed
406
[reference issues]: https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#closing-issues-automatically
407
[Code of Conduct]: https://www.freedesktop.org/wiki/CodeOfConduct/
408
409
[How to Write a Git Commit Message]: https://chris.beams.io/posts/git-commit/
[kernel style]: https://www.kernel.org/doc/Documentation/process/coding-style.rst