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

[META] Add a CONTRIBUTING.md #674

Merged
merged 7 commits into from
Nov 10, 2018
Merged

Conversation

elinorbgr
Copy link
Contributor

This PR introduces a CONTRIBUTING.md file based on the discussion in #669. This is for now very WIP and intended to serve as a basis for discussion, like an RFC.

Regarding the most fundamental still-open questions:

  • who exactly is a "maintainer" of winit, who has commit right and is willing to dedicate time to winit?
  • For now I've left [META] Feature parity between platforms #252 as the "official list of features", but we might want to make it more formal maybe?

Rendered version of the file: https://github.com/vberger/winit/blob/contributing/CONTRIBUTING.md

@Osspial
Copy link
Contributor

Osspial commented Oct 13, 2018

I feel like an official list of supported/targeted features is important enough to get its own file in root (perhaps SCOPE.md?).

Also, as far as tier-1 features go, there are a few things I'm not seeing on the list in the issue:

  • Gyroscope support.
  • Gamepad/joystick support.
  • Creating child windows relative to the client area of other windows, for popup windows. This is essential for desktop applications, and is finicky without direct API access. IIRC it's even impossible to do on Wayland without direct API access.
  • Disabling windows in favor of child windows (e.g. for save dialogs).

Where do we stand on those? AFAIK there was hesitation about gamepad support, the windowing features haven't received much attention, and gyroscope support hasn't received any attention despite being common on mobile devices.

Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff @vberger, thanks for taking the time to put this together!

Other than a couple typos I'm happy to see this land :)

who exactly is a "maintainer" of winit?

I guess everyone contributing PRs, reviewing PRs, testing them and providing feedback are helping with maintenance in their own way, however as for the "lead" maintainers (those with an M in the table) it is admittedly trickier to hone down a definition.

E.g. I would consider you the clear maintainer of Wayland in that you have both merge access, are a frequent reviewer and contributor and have the most extensive knowledge of the backend.

Myself on the other hand, I feel I have a very jack-of-all-backends position in that I'm vaguely familiar with and have contributed a fair share to all of macOS, Linux and Windows, yet I am certainly not an expert in any of them and wouldn't dare claim the lead M role for any. That said, I have merge access and am willing to review, test and merge (any that I have access to at the time) under the condition that the two maintainer policy is met.

CONTRIBUTING.md Outdated
Wayland, Android, iOS and the web platform via Emscripten).

Most platform expose capabilities that cannot be meaningfully transposed to the others. Winit does not
aim at supporting every single functionnaly of every platform, but rather abstract the set of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

functionnaly -> functionality

CONTRIBUTING.md Outdated

Once your PR is open, you can ask for review by a maintainer of your platform. Winit's merging policy
is that a PR must be approved by at least two maintainers of winit before being merged, including
at least a maintainer of the platoform (a maintainer making a PR themselves counts as approving it).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

platoform -> platform

@mitchmindtree
Copy link
Contributor

@Osspial I like your idea of having a SCOPE.md file! It always takes me longer than I'd like to hunt down the the feature table issue. Having a dedicated file would make it easier to find, review and make updates. Perhaps we could even require that changes to it be mentioned in the CHANGELOG.md.

Re tier 1 features: Maybe we can leave this discussion for a future PR/issue dedicated to this. It might be easy to unintentionally derail @vberger's CONTRIBUTING.md if we start the discussion here :)

@elinorbgr
Copy link
Contributor Author

@Osspial 👍 for making a SCOPE.md file, in which case I suggest we could open a new PR for adding it and moving there discussion of defining the scope of winit. I feel having several discussions on the same thread would make things too complicated to follow for everybody.

@mitchmindtree Do you think it may be worth adding a third level, between M and T wrt to platform access/knowledge? So something like:

  • M is a main maintainer for this platform
  • R can review this platform
  • T can test this platform

In general I think it'd be best to keep the number of levels as low as possible to keep things crystal clear, but I guess 3 levels like this is still reasonably simple.

CONTRIBUTING.md Outdated
Winit aims to provide a generic platform abstracting the main graphic platforms (Windows, MacOS, x11,
Wayland, Android, iOS and the web platform via Emscripten).

Most platform expose capabilities that cannot be meaningfully transposed to the others. Winit does not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/platform/platforms

CONTRIBUTING.md Outdated
Wayland, Android, iOS and the web platform via Emscripten).

Most platform expose capabilities that cannot be meaningfully transposed to the others. Winit does not
aim at supporting every single functionaly of every platform, but rather abstract the set of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/aim at supporting/aim to support
s/functionaly/functionality
s/rather abstract/rather to abstract

CONTRIBUTING.md Outdated
different "support levels":

- Tier 1: features which are in the main scope of winit. They are part of the common API of winit, and
are taken care of by the maintainers, and them not working correctly is considered a bug in winit.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/are taken care of by the maintainers, and them not working correctly is considered a bug in winit./are taken care of by the maintainers. Any part of these features that is not working correctly is considered a bug in winit.

CONTRIBUTING.md Outdated

- Tier 1: features which are in the main scope of winit. They are part of the common API of winit, and
are taken care of by the maintainers, and them not working correctly is considered a bug in winit.
- Tier 2: some platform specific features can be sufficiently fundamental to the platform that winit can
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/platform specific/platform-specific

CONTRIBUTING.md Outdated
When making a code contribution to winit, before opening your pull request, please make sure that:

- you tested your modifications on all the platforms impacted, or if not possible detail which platforms
were not tested, and what should be tested, so that a maintainer or an other contributor can test them
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/an other/another

@sodiumjoe
Copy link
Contributor

Left a few copy notes, I think this is great!

@Osspial
Copy link
Contributor

Osspial commented Oct 16, 2018

@vberger I'm fine with splitting the scope discussion off. If we're doing that, should we move most of the current Scope section to SCOPE.md, or just replace the link with one to the new document?

I also like the idea of a reviewer tier in the maintainer/tester table.

@elinorbgr
Copy link
Contributor Author

@Osspial

I'm fine with splitting the scope discussion off. If we're doing that, should we move most of the current Scope section to SCOPE.md, or just replace the link with one to the new document?

I don't have a strong opinion on this. Probably splitting this part of to a SCOPE.md and just keeping a short summary in CONTRIBUTING.md and linking to the SCOPE.md file for details would be the best approach?

@Osspial
Copy link
Contributor

Osspial commented Oct 24, 2018

@vberger That sounds good. For now though, I'd say to leave that part in and then have the SCOPE.md PR move it out.

@elinorbgr
Copy link
Contributor Author

@Osspial Alright, sounds good to me. Let's forward this and then open a large PR/discussion about defining the exact scope of winit.

@francesca64 Given you've been in charge of winit for some time, what's your opinion on this suggested CONTRIBUTING.md ?

Also, @francesca64 @mitchmindtree and @Osspial , assuming we keep the M/R/T/ levels, how should I fill in the table w.r.t. to platforms for each of you?

@Osspial
Copy link
Contributor

Osspial commented Nov 3, 2018

@vberger For me, it would conservatively be this:

Windows MacOS x11 Wayland Android iOS Emscripten
M (I assume) T T T T

Also, I don't know all that much about Emscripten but I do have the compiler set up for it and it seems to be small enough that if we need a reviewer for it learning about would be a doable task!

@mitchmindtree
Copy link
Contributor

Feel free to put me down as T for X11, Wayland and Windows (though I only have access to windows via a VM).

@francesca64
Copy link
Member

@vberger this CONTRIBUTING.md looks great, though I plan on providing some more detailed feedback in the next day or so.

Windows macOS X11 Wayland Android iOS Emscripten
R M M M M

In an ideal world, I'd only be the main maintainer for X11, so it could be good to have a note saying that macOS, Android, and iOS are still up for grabs.

@Osspial I'd be quite happy if you could take over the Emscripten backend, and I don't just say that because it's the only one I never managed to run. It's definitely the one I know the least about, and it would make the EventLoop 2.0 migration much less stressful for me (I also plan on reviewing your PR for that this week, since I've gotten through the rest of my backlog).

I think you'd do a great job as maintainer of the Windows backend, and I'll email @tomaka about making you a collaborator if he doesn't show up here.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@mtak-
Copy link
Contributor

mtak- commented Nov 8, 2018

I would like to help with iOS/Android. My company has 2 devs (mostly me) actively developing a 2D game/game engine in rust using gfx-ll. I'm currently focused on iOS, and have made some contributions there (not recently). I'm intending to get rid of the longjmp hacks and port it to EventLoop 2.0 as my next task.

I'm up for maintaining iOS, and atleast able to test macos/android, but I haven't messed with that code much.

@elinorbgr
Copy link
Contributor Author

Given all this seems now pretty consensual, I'm going to merge this as it is now.

@Osspial Can you open the SCOPE.md PR, given you seem to already have some ideas for it?

@mtak- Can you then open a new PR adding yourself to the table, so that we can discuss there? I have nothing to oppose you taking the maintainership of the iOS backend, but I'd rather not delay this PR any longer. Thanks for proposing your help!

@elinorbgr elinorbgr merged commit dd52364 into rust-windowing:master Nov 10, 2018
@Osspial Osspial mentioned this pull request Nov 10, 2018
@Osspial
Copy link
Contributor

Osspial commented Nov 10, 2018

@vberger Sure! It's open in #695.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants