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

Refactor main example #3542

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Refactor main example #3542

wants to merge 6 commits into from

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Mar 1, 2024

Follow-up to #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 #3512. @aloucks, does this make it better for you? (Especially the rename from window -> full).

  • Tested on all platforms changed

We already have the `Action` enum that abstracts key and mouse event bindings; on top of that, putting each action handler in its own functions seems excessive.
It is more clear what's happening when you don't have to jump to the bottom of the file to look at the implementation.
@madsmtm madsmtm added the S - docs Awareness, docs, examples, etc. label Mar 1, 2024
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

This is just a giant mess now.

@madsmtm
Copy link
Member Author

madsmtm commented Mar 1, 2024

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.

@kchibisov
Copy link
Member

kchibisov commented Mar 1, 2024

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

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

@madsmtm
Copy link
Member Author

madsmtm commented Mar 1, 2024

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?

@kchibisov
Copy link
Member

kchibisov commented Mar 1, 2024

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.

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

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

I think that would be good indeed.


I do agree that this is not usually how you write code, but this isn't supposed to be a good Rust code example, its supposed to be a good example on how to use Winit (obviously there is a spectrum here). In this sense I believe these changes are an improvement.

Though it is true that GitHub has code navigation, scrolling through the example is definitely easier then jumping back and forth between functions inside of GitHub.

@nixpulvis
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S - docs Awareness, docs, examples, etc.
Development

Successfully merging this pull request may close these issues.

4 participants