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

Drop platform name from extension traits #907

Closed
wants to merge 1 commit into from

Conversation

nvzqz
Copy link
Contributor

@nvzqz nvzqz commented Jun 10, 2019

As mentioned in #459 (comment), I feel as though adding the name of the platform to the Ext traits feels awkward and inconsistent with the Rust ecosystem. Also, these traits are already within their own modules specific to each platform.


  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@elinorbgr
Copy link
Contributor

I believe the reason these extension traits have the platform name in their name is because they are not exclusive and more than one may be imported at the same time.

For example someone may need to have both EventLoopExtDesktop and EventLoopExtUnix in scope, which is not possible if they have the same name (in which case this forces the user to rename the import with something like use ...::EventLoopExt as EventLoopExtDesktop.

@ghost
Copy link

ghost commented Jun 10, 2019

I believe it's possible to use syntax such as use winit::platform::{desktop::EventLoopExt as _, macos::EventLoopExt as _} to bring multiple traits with the same name into scope as of Rust 1.33.

@nvzqz
Copy link
Contributor Author

nvzqz commented Jun 10, 2019

If you don't need to refer to the traits by name, you can just do a glob import (playground).

mod a {
    pub trait Trait {}
}

mod b {
    pub trait Trait {}
}

use a::*;
use b::*;

@felixrabe
Copy link
Contributor

See also previous discussion on #459 on Aug 15, 2018: #459 (comment) and #459 (comment) (at the end).

@felixrabe
Copy link
Contributor

felixrabe commented Jun 10, 2019

My suggestion: Use plain EventLoopExt as the name for exclusive platform-specific traits, and EventLoopDesktopExt (as an example for Desktop) for traits that are available in addition.

It might also make sense to move the EventLoopDesktopExt out of the platform module to reduce confusion?

@felixrabe
Copy link
Contributor

felixrabe commented Jun 10, 2019

One more unsolicited idea concerning a possible module structure here:

mod platform
    mod desktop
        trait EventLoopExt
        trait WindowExt

        mod macos
            trait EventLoopExt: super::EventLoopExt
            trait WindowExt: super::WindowExt

        mod unix (excl. macos, maybe also excl. fuchsia and redox)
            trait EventLoopExt: super::EventLoopExt
            trait WindowExt: super::WindowExt

    mod mobile
        trait EventLoopExt
        trait WindowExt

        mod ios
            :

(Disclosure: At this moment, I haven't looked at or used EL2 code yet, just reacting to issue and PR discussion.)

@nvzqz
Copy link
Contributor Author

nvzqz commented Jun 10, 2019

Another approach (which isn't pretty but just putting it out there) is to have fn run_return and type UserEvent go on each OS extension trait directly, and remove desktop::EventLoopExt. I'm not in favor of this approach but it is an option.

@Osspial
Copy link
Contributor

Osspial commented Jun 10, 2019

This is a pretty bikesheddy change. As such I don't feel all that strongly about it and I'm fine with any solution that addresses the problem @vberger mentioned, but since it's so low-impact I don't see a compelling reason to change it.

@Osspial
Copy link
Contributor

Osspial commented Jun 11, 2019

Hey,

I'd like to apologize for being so dismissive of this change in my comment yesterday. Not much discussion was had over the exact naming conventions for this in the original #459 thread, and we mostly settled on this because nobody proposed anything else. I'm open to changes here, but I'll still warn against getting too heated or spending too much time in this discussion - like many design choices, there isn't really a "right" answer here for the naming convention and it's a relatively minor part of the API to boot.

Regardless, here's my two cents on why I'm fine with the current situation: I like internal consistency in designs. It doesn't make a whole lot of sense to me to have a Desktop suffix on the desktop platform traits while not having a suffix on any of the other platforms, and having that inconsistency would increase the mental overhead for users of our API. Despite its clunkiness, remembering there's a platform suffix for every platform trait is easier than remembering there's a platform suffix on these platform traits but not these other platform traits.

To that end, I'd be okay with either:

  • The current design
  • The suffixless design, in light of use WhateverExt as _.

Both have benefits. Both are slightly clunky. I don't feel that strongly about it, and am willing to go with whatever develops a consensus, but if no consensus develops we're just going to default to the current solution.

@Osspial Osspial changed the base branch from eventloop-2.0 to master June 13, 2019 05:27
@goddessfreya
Copy link
Contributor

Closing for inactivity.

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.

5 participants