Skip to content

winit-core: new crate #4208

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
May 14, 2025
Merged

winit-core: new crate #4208

merged 18 commits into from
May 14, 2025

Conversation

@kchibisov
Copy link
Member

@kchibisov kchibisov commented Apr 29, 2025 •
edited by madsmtm
Loading

Split trait based public API of winit into winit-core. Part of #3433.

--

Draft for now, since need to take a pass on docs and organize where things should lay.

madsmtm
madsmtm reviewed Apr 29, 2025
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Quick look so far, very focused on making reviewing this easier.

@madsmtm madsmtm added this to the Version 0.31.0 milestone Apr 29, 2025
@kchibisov kchibisov force-pushed the kchibisov/split-core branch 9 times, most recently from 12e2587 to 42c9a07 Compare May 3, 2025 13:17
@kchibisov kchibisov marked this pull request as ready for review May 3, 2025 13:39
@kchibisov kchibisov requested a review from madsmtm May 3, 2025 13:39
@kchibisov
Copy link
Member Author

kchibisov commented May 3, 2025

I've split it somehow logically. Also, I haven't added everything to public API since I made a bunch of constructors pub. Though, they are not that relevant for the users, since they can not do much with them.

I'll still go through and add missing.

Also, not sure what to do with platform specific docs in winit-core.

@kchibisov
Copy link
Member Author

kchibisov commented May 3, 2025

Also, the WindowAttributes and the platform part on it I'm not that found of, it does kind of what it should, but I feel like there could be a better way...

@kchibisov
Copy link
Member Author

kchibisov commented May 3, 2025

The API will also need non_exhaustive throughout, but I think that and other tuning of such sort are for the later PRs.

madsmtm
madsmtm requested changes May 4, 2025
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I reviewed it commit-by-commit, and then globally, it was very clear to follow. Though I think we should probably still squash-merge, for better git history (again, very much my focus here).

Requesting changes to:

  • Figure out the docs stuff.
  • Perhaps move a few things to separate PRs.
  • Discuss avoiding constructors on POD types like VideoMode.

@kchibisov
Copy link
Member Author

kchibisov commented May 5, 2025

Also, the intend is not squash anything, since every commit builds on its own, so while I can take things out, some things don't make sense without the rest of changes here e.g. window attributes.

@kchibisov kchibisov force-pushed the kchibisov/split-core branch from 42c9a07 to 99831d5 Compare May 5, 2025 12:35
@kchibisov kchibisov requested a review from madsmtm May 5, 2025 12:39
@madsmtm madsmtm mentioned this pull request May 5, 2025
Merged
@madsmtm
Copy link
Member

madsmtm commented May 5, 2025

I'm okay with rebase-merging, but only if you squash "winit-core: move window, except attrs", "winit-core: move WindowAttributes" and "winit: drop window module stub". Again, I want git blame winit-core/src/window.rs to work.

@kchibisov kchibisov force-pushed the kchibisov/split-core branch from 99831d5 to 355e500 Compare May 5, 2025 13:52
@kchibisov
Copy link
Member Author

kchibisov commented May 5, 2025

I'm okay with rebase-merging, but only if you squash "winit-core: move window, except attrs", "winit-core: move WindowAttributes" and "winit: drop window module stub". Again, I want git blame winit-core/src/window.rs to work.

squashed them.

madsmtm
madsmtm reviewed May 5, 2025
View reviewed changes
@madsmtm
Copy link
Member

madsmtm commented May 5, 2025

The only real blocker for me now is that the EventLoop stuff is a fair bit of git blame that will be lost, and that'd be unfortunate if the plan is to move it to winit-core anyhow at a later date.

@madsmtm madsmtm mentioned this pull request May 12, 2025
Open
madsmtm
madsmtm approved these changes May 13, 2025
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

#4208 (comment) is still open, but otherwise I'm good to move forwards with this.

@madsmtm madsmtm added the S - api Design and usability label May 13, 2025
@kchibisov kchibisov force-pushed the kchibisov/split-core branch from 355e500 to 09038bc Compare May 14, 2025 12:09
kchibisov added 18 commits May 14, 2025 21:09
@kchibisov
ci/deny: allow scripts in zerocopy
09c551b
@kchibisov
winit-core: new crate + split out as_any
3c3e3e6
@kchibisov
winit-core: move monitor handle
641be83
@kchibisov
winit-core: move keyboard
2b4c9ab
@kchibisov
winit-core: move icon
6187443
@kchibisov
winit-core: move cursor
7627054
@kchibisov
winit-core: move error
291a7bd
@kchibisov
winit-core: partially split event_loop
c695f77
@kchibisov
winit-core: move window
001076c 
Create `WindowAttributes` for respective platform specific window
attributes in `winit` due to move of `WindowAttributes`.
@kchibisov
winit-core: move ActiveEventLoop
ec4336b
@kchibisov
winit-core: move event
1d754dd
@kchibisov
winit-core: move application
c387cde
@kchibisov
winit-core: cleanup event loop docs
51bcecc
@kchibisov
winit-core: drop broken docs
17089fb 
Generally, winit-core doesn't know about underlying platforms, though,
some general information which will true for any implementation was
left in place.
@kchibisov
winit-core: drop all cfg except web
e0d2f98
@kchibisov
winit-core: fix tests
5c66cd3
@kchibisov
ci: test winit-core
4eb5203
@kchibisov
winit-core: add top-level doc
f320fb2
@kchibisov kchibisov force-pushed the kchibisov/split-core branch from 09038bc to f320fb2 Compare May 14, 2025 12:10
@kchibisov kchibisov merged commit b5921d8 into master May 14, 2025
57 checks passed
@kchibisov kchibisov deleted the kchibisov/split-core branch May 14, 2025 12:18
@madsmtm
Copy link
Member

madsmtm commented May 14, 2025

Thanks for pushing forwards on this @kchibisov!

@madsmtm madsmtm mentioned this pull request May 15, 2025
Closed
5 tasks
@madsmtm madsmtm mentioned this pull request Jun 5, 2025
Merged
@madsmtm madsmtm mentioned this pull request Sep 4, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@madsmtm madsmtm madsmtm approved these changes

@notgull notgull notgull approved these changes

@jackpot51 jackpot51 Awaiting requested review from jackpot51

@daxpedda daxpedda Awaiting requested review from daxpedda

@MarijnS95 MarijnS95 Awaiting requested review from MarijnS95

+1 more reviewer

@PoignardAzur PoignardAzur PoignardAzur left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

S - api Design and usability

Milestone

Version 0.31.0

Development

Successfully merging this pull request may close these issues.

5 participants

@kchibisov @madsmtm @PoignardAzur @notgull