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

FrameTimings: use an `Option` when returning presentation time #251

Merged
merged 1 commit into from Oct 17, 2018

Conversation

Projects
None yet
4 participants
@fengalin
Copy link
Contributor

fengalin commented Oct 14, 2018

get_presentation_time and get_predicted_presentation_time can both return 0 when the returned value is not available. In Rust, using an Option and returning None would make it clear that the value shouldn't be used as is.

See #250

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 14, 2018

Please update gir submodule too or do regen with gir c385982dbed5fdbdf1ae6f95c885a475cafc93f6

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 14, 2018

Other that this nit all LFGM

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Oct 14, 2018

Oh! Sure, ' will do that.

@fengalin fengalin force-pushed the fengalin:presentation_time_option branch from ae7e4ab to fbb0001 Oct 14, 2018

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Oct 14, 2018

Fixed, sorry.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 14, 2018

@fengalin Thanks.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 14, 2018

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 14, 2018

Should probably be an Option<NonZero<_>> then?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 14, 2018

Are negative values valid?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 15, 2018

Are negative values valid?

I'll wait for this answer before merging. :)

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Oct 15, 2018

Should probably be an Option<NonZero<_>> then?

I'm not sure about this. I agree this would be a nice addition, but searching for NonZero in the std, I could only spot usigned integers candidates.

Are negative values valid?

It's not clear to me whether the timings might be negative. However, the GDK code seems to always make use of if (presentation_time != 0) when handling similar cases, so I wouldn't take the risk of assuming they are exclusively positive.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 15, 2018

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Oct 15, 2018

Maybe https://crates.io/crates/nonzero_signed :)

Neat! Thanks, I'll do that ASAP.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 15, 2018

A question on this: should we add a crate this small as a dependency or should we just write the needed type and go for it? We already have quite a lot of dependencies after all and the rendering of the docs won't appreciate much (I just keep "top-level" crates).

What do you think of this?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 15, 2018

I'd just depend on it, that's what crates are for. If rustdoc has problems with many dependencies then that's a bug in rustdoc that has to be fixed there.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 15, 2018

Not a bug in rustdoc, a "bug" in our documentation generation.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 15, 2018

What exactly is the problem?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 15, 2018

Not really a problem, just a nuisance. I'll just have to update the release script, but from now on, we will have to add the crate to gtk-rs docs.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 15, 2018

👍

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 15, 2018

I still think it's an API bug that this is not a (unsigned!) guint64 on the C side. Negative values make no sense really, or 0 would make equally much sense as a valid value :)

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Oct 15, 2018

I just realized I didn't conditioned the dependency on NoneZeroI64 to the proper version... Doing it now.

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Oct 15, 2018

I still think it's an API bug that this is not a (unsigned!) guint64 on the C side. Negative values make no sense really, or 0 would make equally much sense as a valid value :)

My guess is that it returns a signed in order to ease arithmetic operations, but I agree timings shouldn't be negative.

Do you think we should fix this in the Rust binding?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 15, 2018

We could, yes. Casting is annoying in Rust and implicit in C

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Oct 15, 2018

All right, I kept the commit stack for the record. I can squash if everyone agrees with the unsigned approach.

ffi::gdk_frame_timings_get_predicted_presentation_time(self.to_glib_none().0)
pub fn get_predicted_presentation_time(&self) -> Option<NonZeroU64> {
NonZeroU64::new(unsafe {
ffi::gdk_frame_timings_get_predicted_presentation_time(self.to_glib_none().0) as u64

This comment has been minimized.

@sdroege

sdroege Oct 16, 2018

Member

Maybe add an assertion here for negative numbers, and below

This comment has been minimized.

@fengalin

fengalin Oct 16, 2018

Author Contributor

Of course!

@fengalin fengalin force-pushed the fengalin:presentation_time_option branch from 0134159 to a9af748 Oct 16, 2018

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Oct 16, 2018

@EPashkin & @GuillaumeGomez, are you ok with latest version?

Edit: if so, I'll squash.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 16, 2018

@fengalin Yes, Thanks

FrameTimings: use an `Option` when returning presentation time
`get_presentation_time` and `get_predicted_presentation_time` can both
return `0` when the returned value is not available. In Rust, using an
`Option` and returning `None` would make it clear that the value
shouldn't be used as is.
@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 16, 2018

Yep, all good for me, thanks! Ping me once squash is done and then I merge!

@fengalin fengalin force-pushed the fengalin:presentation_time_option branch from a9af748 to 05f7e3a Oct 17, 2018

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Oct 17, 2018

@GuillaumeGomez: squashed and all green now! :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 17, 2018

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 5217eb0 into gtk-rs:master Oct 17, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

fengalin added a commit to fengalin/media-toc that referenced this pull request Oct 17, 2018

@fengalin fengalin deleted the fengalin:presentation_time_option branch Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.