Add optional Serde implementations and missing derivable traits #652

Osspial · 2018-09-12T21:34:08Z

This is useful for things like configuration files, especially for saving keyboard/mouse bindings and such. This addresses #304.

Implementations are added for:

All types in dpi.
KeyboardInput
ElementState
MouseButton
MouseScrollDelta
VirtualKeyCode
ModifiersState
ControlFlow
MouseCursor
WindowAttributes

This PR also fills in a couple of API holes where some types were missing standard derivable implementations (PartialEq, Eq, and Hash), though that can be moved out of this PR if we decide we don't want to add Serde support.

elinorbgr · 2018-09-14T07:44:33Z

src/lib.rs

@@ -419,6 +426,8 @@ pub struct WindowAttributes {
    /// Whether the window should be set as fullscreen upon creation.
    ///
    /// The default is `None`.
+    #[cfg_attr(feature = "serde", doc("\nThis field does not currently get serialized or deserialized."))]
+    #[cfg_attr(feature = "serde", serde(skip))] // TODO: FIGURE OUT HOW TO SERIALIZE THIS
    pub fullscreen: Option<MonitorId>,


I'm not sure this can be serialized in any meaningful way. This would require that the monitors have a consistent ids as seen by the app between two executions. Do any platform provide this kind of guarantee ?

I can't say I know enough about the platform-specific APIs to give this a firm answer. However, I'd be really surprised if they didn't - being able to enumerate over a consistent list of monitors and choose the right one is really important on multi monitor setups, especially since the first monitor reported isn't always the one users want your application to be on.

Since you're bringing this up, does Wayland not provide APIs to do this consistently?

To be honest I'm not sure.

On the one hand there are the global ids, that uniquely identify a monitor, but have no guarantee to be stable across reboots, not even to still be valid at all.

On the other hand, the wayland server reports the "name" of the monitor (in the form of model an make), which is stable across reboots, but are not unique identifiers (two monitors of the same model would have the same name I believe).

On macOS, display IDs (get_native_identifier) aren't consistent across reboots, and get_name gives you the model number which appears to be quite short (i.e. my main monitor is 1046), so it probably isn't that unique, though it's presumably consistent. We'd probably have to use IOKit to get better info.

HMONITOR on Windows is just a pointer. get_name isn't the name of the hardware model, but rather contains a number for the display (i.e. \\\\.\\DISPLAY1). get_native_identifier returns the same value as get_name here. After doing some testing, the numbers are initially assigned presumably based on port order, and are consistent across reboots. However, any monitors added/replugged after booting are assigned numbers based on the order they were added in, and will fill any gaps in the numerical sequence. That numbering could be different from what will be assigned after rebooting. By replugging multiple monitors at once, it's also possible to rearrange the numbers in arbitrary ways, which wouldn't be conserved either. Note that these numbers appear to have no relation to the numbers shown in the display settings.

On X11, get_name gives you the name+index of which port the monitor is using, which should actually be adequate for most purposes (i.e. my Nvidia configuration uses them to identify each monitor). get_native_identifier just gives you the index from enumerating the monitor list, which is presumably fairly worthless.

This is a non-issue on non-desktop platforms, where MonitorId only exists for the sake of uniformity.

Hmm. Is the order in which displays are reported consistent across reboots? If that's the case, we could see about serializing an index into the reported display list, although that would be rather hacky.

If we can't find a good solution for this soonish, I'd say that we should make a separate issue for figuring out how to proceed and then merge this as-is.

I think leaving this as a separate issue is probably a good approach here.

I'm okay with that.

Osspial · 2018-09-17T21:35:30Z

Is there anything blocking this from getting merged? @tomaka, you gave this a thumbs-down; is there any particular reason you object to this?

Osspial · 2018-09-23T18:16:50Z

...?

Osspial · 2018-10-06T16:01:46Z

@francesca64 @tomaka

Unless there are any objections, I don't think there's anything blocking this from getting merged.

mitchmindtree · 2018-10-07T16:25:09Z

Some trade-offs that come to mind when adding serde support are:

It makes it slightly trickier to add new fields. E.g. To avoid breakage, all new fields in the future must also be serializable and have reasonable defaults for deserialization, which may not always make sense (WindowAttributes in particular comes to mind).
Adds another feature and dependency to the maintainership burden.
Adds another build feature variation to be tested in CI. Speaking of, maybe this PR should add a line to the .travis.yml for this?

I'm really not sure what the right decision is with this PR, I can sympathise with the desire to have this for configuration (particularly the VirtualKeyCode type - it would be very annoying to have to copy that enum just to serialize it and write a conversion function between it and the winit type). I personally think the WindowAttributes is a bit much, mostly as it contains strange platform-specific types like MonitorId and as a result I'd imagine it might be better for the downstream user to create their own config type for serialization anyway. Sorry for the indecisiveness, just thought I'd add my thoughts on why there might be some hesitation.

jwilm · 2018-10-24T21:14:53Z

Alacritty copies the VirtualKeyCode enum, derives serde traits on it, and adds a conversion with the actual winit type. We would love to see serde support for this type!

ekse · 2018-10-24T22:13:36Z

Another vote in favor of the conversion of VirtualKeyCode for the veloren project, see rust-windowing/glutin#1074.

Osspial · 2018-10-24T23:07:36Z

@mitchmindtree I would be alright with removing the WindowAttributes implementation. I don't think there are any other types where implementing serialization traits is a real issue (except maybe ControlFlow, but I don't think that's a big deal).

elinorbgr · 2018-10-25T07:47:59Z

CHANGELOG.md

@@ -21,6 +21,8 @@
 - On X11, now a `Resized` event will always be generated after a DPI change to ensure the window's logical size is consistent with the new DPI.
 - Added further clarifications to the DPI docs.
 - On Linux, if neither X11 nor Wayland manage to initialize, the corresponding panic now consists of a single line only.
+- Add optional `serde` feature with implementations of `Serialize`/`Deserialize` for DPI types, various event types, and `WindowAttributes`.


Given this PR does not longer derive serialize traits for WindowAttributes, this changelog line should be updated.

@Osspial once you take care of this, we'll be good to merge.

elinorbgr · 2018-10-25T07:51:20Z

@mitchmindtree

maybe this PR should add a line to the .travis.yml for this?

I'd suggest adding a travis lines to test this, as well as a test file that asserts that the types inquestion do indeed implement the serialize/deserialize traits, to ease future maintenance/refactors/whatever should happen.

francesca64 · 2018-10-29T02:27:48Z

Sorry for not weighing in sooner, but I'm in support of this PR (and agree with the choice to exclude WindowAttributes). I don't see any serious downsides, and having this functionality is undeniably convenient.

If @tomaka has any objection, then I'd assume it's the same as in rust-windowing/glutin#842 (comment). I personally don't see any issue with having an optional dependency on serde, and we already have an optional dependency on image anyway. So, since @tomaka has never shown up to argue against this, I think it's reasonable to go ahead with merging this.

Osspial · 2018-10-29T22:11:25Z

Test lines/files have been added. I also added in lines for the icon_loading feature, since they didn't seem to be in there.

Osspial · 2018-10-29T22:19:53Z

...huh, the icon_loading feature breaks 1.24.1.

francesca64 · 2018-10-29T22:42:59Z

Oh gosh! Since we didn't have icon_loading tested on CI before, #683 must've broken 1.24.1 compatibility without me realizing.

From looking at the logs, it would be easy to make image 0.20.0 work on 1.24.1. image doesn't pin to any version currently, and image-rs/image#482 already exists for discussing that. I'll take care of that either today or tomorrow, though it's not something that blocks this PR.

francesca64 · 2018-11-01T08:24:46Z

Travis failure caused by rust-lang/rust#55571

elinorbgr reviewed Sep 14, 2018

View reviewed changes

francesca64 mentioned this pull request Oct 24, 2018

Proposal: Conversion of VirtualKeyCode to string rust-windowing/glutin#1074

Closed

Osspial added 7 commits October 24, 2018 19:14

Add optional serde feature

cf7df03

Document features in README

a81c8e8

Add changelog entry

bce5ae0

Implement some missing derivable traits

245b768

Add changelog entry for std derives

c6b82b5

Remove extraneous space on serde doc comments

621253a

Add period to end of serde line in readme

e18e8ee

Osspial force-pushed the serde_impl branch from d92bc1f to e18e8ee Compare October 24, 2018 23:15

Remove serde impls from WindowAttributes

c3f3ddd

elinorbgr reviewed Oct 25, 2018

View reviewed changes

Osspial added 3 commits October 29, 2018 18:05

Add serde impls for TouchPhase

5804a21

Add serde test file

f2d1185

Add feature lines to testing CIs

300c7cc

Osspial mentioned this pull request Oct 31, 2018

version 0.18.0 #680

Merged

Remove WindowAttributes from changelog

13cfcd8

francesca64 merged commit 6bec912 into rust-windowing:master Nov 1, 2018

francesca64 mentioned this pull request Nov 12, 2018

Configuration file support #304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional Serde implementations and missing derivable traits #652

Add optional Serde implementations and missing derivable traits #652

Osspial commented Sep 12, 2018

elinorbgr Sep 14, 2018

Osspial Sep 14, 2018 •

edited

Loading

elinorbgr Sep 14, 2018

francesca64 Sep 14, 2018

Osspial Sep 14, 2018

elinorbgr Sep 16, 2018

Osspial Sep 17, 2018

Osspial commented Sep 17, 2018

Osspial commented Sep 23, 2018

Osspial commented Oct 6, 2018

mitchmindtree commented Oct 7, 2018

jwilm commented Oct 24, 2018

ekse commented Oct 24, 2018

Osspial commented Oct 24, 2018

elinorbgr Oct 25, 2018

francesca64 Nov 1, 2018

elinorbgr commented Oct 25, 2018

francesca64 commented Oct 29, 2018 •

edited

Loading

Osspial commented Oct 29, 2018

Osspial commented Oct 29, 2018

francesca64 commented Oct 29, 2018

francesca64 commented Nov 1, 2018

Add optional Serde implementations and missing derivable traits #652

Add optional Serde implementations and missing derivable traits #652

Conversation

Osspial commented Sep 12, 2018

Choose a reason for hiding this comment

Osspial Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Osspial commented Sep 17, 2018

Osspial commented Sep 23, 2018

Osspial commented Oct 6, 2018

mitchmindtree commented Oct 7, 2018

jwilm commented Oct 24, 2018

ekse commented Oct 24, 2018

Osspial commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elinorbgr commented Oct 25, 2018

francesca64 commented Oct 29, 2018 • edited Loading

Osspial commented Oct 29, 2018

Osspial commented Oct 29, 2018

francesca64 commented Oct 29, 2018

francesca64 commented Nov 1, 2018

Osspial Sep 14, 2018 •

edited

Loading

francesca64 commented Oct 29, 2018 •

edited

Loading