winit icon indicating copy to clipboard operation
winit copied to clipboard

Refactor main example

Open madsmtm opened this issue 1 year ago • 5 comments

Follow-up to https://github.com/rust-windowing/winit/pull/3447, since I didn't really review the example back then. See each commit for details.

Should also resolve some of the concerns raised in https://github.com/rust-windowing/winit/issues/3512. @aloucks, does this make it better for you? (Especially the rename from window -> full).

  • [ ] Tested on all platforms changed

madsmtm avatar Mar 01 '24 08:03 madsmtm

This is just a giant mess now.

Which commits in particular are you dissatisfied with?

This isn't code like I'd normally write it, but I believe it's better this way as example code, since it's easier to jump in to.

madsmtm avatar Mar 01 '24 08:03 madsmtm

I don't like to advise to write or suggest garbage code

1edf4f123885a12590eef3c2b3e7faef8e6cb772 (makes the code less readable) c6dfe620ff093ed1d10dc10d7c7799f34d8500a8 (will start format like garbage iirc) 21fecf2cb23e7be5a76f42ff5cde3ec26d233888 (even worse code which is hard to follow, code is less dense now) 926fb051de17ec6cac291e69a8e87c0e127bebda (there always should be a window example)

kchibisov avatar Mar 01 '24 09:03 kchibisov

I don't like to advise to write or suggest garbage code

1edf4f1 (makes the code less readable)

I disagree, I think it becomes much more readable, especially when you're new to the codebase, and don't know what things mean. More specifically, what is it that you dislike? Is it the lack of encapsulation (Application is accessing WindowState's fields directly) or the length of the handle_action function?

Remember, we also have to optimize for people without rust-analyzer set up, most people are going to be viewing this on GitHub.

c6dfe62 (will start format like garbage iirc)

The functionality is unchanged in that commit, the Display impl was just forwarding to Debug.

21fecf2 (even worse code which is hard to follow, code is less dense now)

I can agree that this one may be dubious, though I do still think it improves first-time readability.

926fb05 (there always should be a window example)

Then let's add another example called window, that instead does the bare minimum?

madsmtm avatar Mar 01 '24 09:03 madsmtm

Remember, we also have to optimize for people without rust-analyzer set up, most people are going to be viewing this on GitHub.

github has code navigation for more than a year. I don't even use rust analyzer to navigate any of that as well.

Then let's add another example called window, that instead does the bare minimum?

Then you should do that in the first place.

I can agree that this one may be dubious, though I do still think it improves first-time readability.

No, no one cares about giant functions on how to pick location or how all the cursors are named in the example, it's just noise and doesn't show how to do things.

I'd agree to inline the WindowState creation and remove its new, and split monitors stuff, but that's about it. I don't care about rename as long as we have window.rs example. The new example should be named app.rs

And no, this won't solve the linked issue because it's completely irrelevant to what you did and how it should be solved.

kchibisov avatar Mar 01 '24 09:03 kchibisov

As someone who is still somewhat unfamiliar with winit, I find this example pretty readable and helpful. The only functions that seem complex are Application::new and Application::create_window, everything else seems like reasonable boilerplate for a full fledged application.

I do think having a more minimal example would help people looking to build quickly from the ground up though, which is usually what I look to do when learning a new library.

nixpulvis avatar May 07 '24 17:05 nixpulvis