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.

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

Remove PartialOrd and Ord implementations from WindowId, DeviceId, MonitorHandle and VideoModeHandle #3866

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

Remove PartialOrd and Ord implementations from WindowId, DeviceId, MonitorHandle and VideoModeHandle #3866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9a4e0b
Remove non-meaningful implementations of `Ord`
daxpedda Aug 14, 2024
6a12fa1
Address review
daxpedda Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/changelog/unreleased.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ changelog entry.
- Remove `MonitorHandle::size()` and `refresh_rate_millihertz()` in favor of
`MonitorHandle::current_video_mode()`.
- On Android, remove all `MonitorHandle` support instead of emitting false data.
- Remove `PartialOrd` and `Ord` implementations from `WindowId`, `DeviceId`, `MonitorHandle` and
`VideoModeHandle`.

### Fixed

Expand Down
11 changes: 3 additions & 8 deletions src/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ pub enum WindowEvent {
/// `DeviceId` which identifies its origin. Note that devices may be virtual (representing an
/// on-screen cursor and keyboard focus) or physical. Virtual devices typically aggregate inputs
/// from multiple physical devices.
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct DeviceId(pub(crate) platform_impl::DeviceId);

impl Default for DeviceId {
Expand All @@ -457,7 +457,7 @@ impl DeviceId {
///
/// Whenever a touch event is received it contains a `FingerId` which uniquely identifies the finger
/// used for the current interaction.
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct FingerId(pub(crate) platform_impl::FingerId);

impl FingerId {
Expand Down Expand Up @@ -1032,7 +1032,7 @@ impl Eq for InnerSizeWriter {}

#[cfg(test)]
mod tests {
use std::collections::{BTreeSet, HashSet};
use std::collections::HashSet;

use crate::dpi::PhysicalPosition;
use crate::event;
Expand Down Expand Up @@ -1166,11 +1166,6 @@ mod tests {
let did = crate::event::DeviceId::dummy().clone();
let fid = crate::event::FingerId::dummy().clone();
HashSet::new().insert(did);
let mut set = [did, did, did];
set.sort_unstable();
let mut set2 = BTreeSet::new();
set2.insert(did);
set2.insert(did);

HashSet::new().insert(event::TouchPhase::Started.clone());
HashSet::new().insert(event::MouseButton::Left.clone());
Expand Down
23 changes: 1 addition & 22 deletions src/monitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,27 +24,6 @@ impl std::fmt::Debug for VideoModeHandle {
}
}

impl PartialOrd for VideoModeHandle {
fn partial_cmp(&self, other: &VideoModeHandle) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for VideoModeHandle {
fn cmp(&self, other: &VideoModeHandle) -> std::cmp::Ordering {
self.monitor().cmp(&other.monitor()).then(
self.size()
.cmp(&other.size())
.then(
self.refresh_rate_millihertz()
.cmp(&other.refresh_rate_millihertz())
.then(self.bit_depth().cmp(&other.bit_depth())),
)
.reverse(),
Comment on lines -35 to -43
Copy link
Member

Choose a reason for hiding this comment

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

I agree that VideoModeHandle's Ord is weird (and likely incorrect, I could imagine two video modes that have the same monitor, size, refresh rate and bit depth, but differed on some other parameter, in which case Ord would say they were equal, but Eq would say they weren't).

)
}
}

impl VideoModeHandle {
/// Returns the resolution of this video mode. This **must not** be used to create your
/// rendering surface. Use [`Window::inner_size()`] instead.
Expand Down Expand Up @@ -112,7 +91,7 @@ impl std::fmt::Display for VideoModeHandle {
/// to check.
///
/// [`Window`]: crate::window::Window
#[derive(Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
#[derive(Clone, PartialEq, Eq, Hash)]
pub struct MonitorHandle {
pub(crate) inner: platform_impl::MonitorHandle,
}
Expand Down
8 changes: 4 additions & 4 deletions src/platform_impl/android/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,7 +666,7 @@ impl OwnedDisplayHandle {
}
}

#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
pub(crate) struct WindowId;

impl WindowId {
Expand All @@ -687,7 +687,7 @@ impl From<u64> for WindowId {
}
}

#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
pub struct DeviceId(i32);

impl DeviceId {
Expand All @@ -696,7 +696,7 @@ impl DeviceId {
}
}

#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
pub struct FingerId(i32);

impl FingerId {
Expand Down Expand Up @@ -947,7 +947,7 @@ impl Display for OsError {
}
}

#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Clone, Debug, PartialEq, Eq, Hash)]
pub struct MonitorHandle;

impl MonitorHandle {
Expand Down
4 changes: 2 additions & 2 deletions src/platform_impl/apple/appkit/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use crate::event::DeviceId as RootDeviceId;
pub(crate) use crate::icon::NoIcon as PlatformIcon;
pub(crate) use crate::platform_impl::Fullscreen;

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct DeviceId;

impl DeviceId {
Expand All @@ -43,7 +43,7 @@ impl DeviceId {
// Constant device ID; to be removed when if backend is updated to report real device IDs.
pub(crate) const DEVICE_ID: RootDeviceId = RootDeviceId(DeviceId);

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct FingerId;

impl FingerId {
Expand Down
15 changes: 0 additions & 15 deletions src/platform_impl/apple/appkit/monitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,21 +147,6 @@ impl PartialEq for MonitorHandle {

impl Eq for MonitorHandle {}

impl PartialOrd for MonitorHandle {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for MonitorHandle {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
unsafe {
ffi::CGDisplayCreateUUIDFromDisplayID(self.0)
.cmp(&ffi::CGDisplayCreateUUIDFromDisplayID(other.0))
}
}
}

impl std::hash::Hash for MonitorHandle {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
unsafe {
Expand Down
2 changes: 1 addition & 1 deletion src/platform_impl/apple/appkit/window.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ impl Window {
}
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct WindowId(pub usize);

impl WindowId {
Expand Down
4 changes: 2 additions & 2 deletions src/platform_impl/apple/uikit/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub(crate) use crate::platform_impl::Fullscreen;
/// UIKit (i.e. you can't differentiate between different external keyboards,
/// or whether it was the main touchscreen, assistive technologies, or some
/// other pointer device that caused a touch event).
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct DeviceId;

impl DeviceId {
Expand All @@ -37,7 +37,7 @@ impl DeviceId {

pub(crate) const DEVICE_ID: RootDeviceId = RootDeviceId(DeviceId);

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct FingerId(usize);

impl FingerId {
Expand Down
25 changes: 4 additions & 21 deletions src/platform_impl/apple/uikit/monitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![allow(clippy::unnecessary_cast)]

use std::collections::{BTreeSet, VecDeque};
use std::collections::VecDeque;
use std::num::{NonZeroU16, NonZeroU32};
use std::{fmt, hash, ptr};

Expand All @@ -12,7 +12,6 @@ use objc2_ui_kit::{UIScreen, UIScreenMode};

use super::app_state;
use crate::dpi::{PhysicalPosition, PhysicalSize};
use crate::monitor::VideoModeHandle as RootVideoModeHandle;

// Workaround for `MainThreadBound` implementing almost no traits
#[derive(Debug)]
Expand Down Expand Up @@ -113,19 +112,6 @@ impl PartialEq for MonitorHandle {

impl Eq for MonitorHandle {}

impl PartialOrd for MonitorHandle {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for MonitorHandle {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
// TODO: Make a better ordering
(self as *const Self).cmp(&(other as *const Self))
}
}

impl fmt::Debug for MonitorHandle {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("MonitorHandle")
Expand Down Expand Up @@ -183,17 +169,14 @@ impl MonitorHandle {
pub fn video_modes(&self) -> impl Iterator<Item = VideoModeHandle> {
run_on_main(|mtm| {
let ui_screen = self.ui_screen(mtm);
// Use Ord impl of RootVideoModeHandle

let modes: BTreeSet<_> = ui_screen
let modes: Vec<_> = ui_screen
.availableModes()
.into_iter()
.map(|mode| RootVideoModeHandle {
video_mode: VideoModeHandle::new(ui_screen.clone(), mode, mtm),
})
.map(|mode| VideoModeHandle::new(ui_screen.clone(), mode, mtm))
.collect();

modes.into_iter().map(|mode| mode.video_mode)
modes.into_iter()
})
}

Expand Down
2 changes: 1 addition & 1 deletion src/platform_impl/apple/uikit/window.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,7 @@ impl Inner {
}
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct WindowId {
window: *mut WinitUIWindow,
}
Expand Down
8 changes: 4 additions & 4 deletions src/platform_impl/linux/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ pub(crate) enum Window {
Wayland(wayland::Window),
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct WindowId(u64);

impl From<WindowId> for u64 {
Expand All @@ -161,7 +161,7 @@ impl WindowId {
}
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub enum DeviceId {
#[cfg(x11_platform)]
X(x11::DeviceId),
Expand All @@ -178,7 +178,7 @@ impl DeviceId {
}
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub enum FingerId {
#[cfg(x11_platform)]
X(x11::FingerId),
Expand All @@ -195,7 +195,7 @@ impl FingerId {
}
}

#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum MonitorHandle {
#[cfg(x11_platform)]
X(x11::MonitorHandle),
Expand Down
4 changes: 2 additions & 2 deletions src/platform_impl/linux/wayland/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ impl From<WaylandError> for OsError {
}

/// Dummy device id, since Wayland doesn't have device events.
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct DeviceId;

impl DeviceId {
Expand All @@ -71,7 +71,7 @@ impl DeviceId {
}
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct FingerId(i32);

impl FingerId {
Expand Down
12 changes: 0 additions & 12 deletions src/platform_impl/linux/wayland/output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,18 +86,6 @@ impl PartialEq for MonitorHandle {

impl Eq for MonitorHandle {}

impl PartialOrd for MonitorHandle {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for MonitorHandle {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.native_identifier().cmp(&other.native_identifier())
}
}

impl std::hash::Hash for MonitorHandle {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.native_identifier().hash(state);
Expand Down
4 changes: 2 additions & 2 deletions src/platform_impl/linux/x11/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,7 @@ impl<'a> Deref for DeviceInfo<'a> {
}
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct DeviceId(xinput::DeviceId);

impl DeviceId {
Expand All @@ -817,7 +817,7 @@ impl DeviceId {
}
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct FingerId(u32);

impl FingerId {
Expand Down
12 changes: 0 additions & 12 deletions src/platform_impl/linux/x11/monitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,18 +76,6 @@ impl PartialEq for MonitorHandle {

impl Eq for MonitorHandle {}

impl PartialOrd for MonitorHandle {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for MonitorHandle {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.id.cmp(&other.id)
}
}

impl std::hash::Hash for MonitorHandle {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.id.hash(state);
Expand Down
8 changes: 4 additions & 4 deletions src/platform_impl/orbital/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ impl TimeSocket {
#[derive(Default, Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub(crate) struct PlatformSpecificEventLoopAttributes {}

#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
pub struct WindowId {
fd: u64,
}
Expand All @@ -119,7 +119,7 @@ impl From<u64> for WindowId {
}
}

#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
pub struct DeviceId;

impl DeviceId {
Expand All @@ -128,7 +128,7 @@ impl DeviceId {
}
}

#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
pub struct FingerId;

impl FingerId {
Expand Down Expand Up @@ -193,7 +193,7 @@ pub(crate) use crate::cursor::{
};
pub(crate) use crate::icon::NoIcon as PlatformIcon;

#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
pub struct MonitorHandle;

impl MonitorHandle {
Expand Down
Loading
Loading