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 refresh_interval #252

Merged
merged 1 commit into from Oct 18, 2018

Conversation

Projects
None yet
4 participants
@fengalin
Copy link
Contributor

fengalin commented Oct 17, 2018

gdk_frame_timings_get_refresh_interval returns 0 when the value is not available.

Same as #251.

FrameTimings: use an `Option` when returning refresh_interval
`gdk_frame_timings_get_refresh_interval` returns `0` when the value
is not available.
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 17, 2018

@fengalin You seems right again, 👍

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Oct 17, 2018

I wish I had seen this as part of #251, but better late than never!

ffi::gdk_frame_timings_get_refresh_interval(self.to_glib_none().0)
};
// assuming refresh interval is always positive
assert!(refresh_interval >= 0);

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 17, 2018

Member

If not positive, shouldn't it return None instead?

This comment has been minimized.

@fengalin

fengalin Oct 18, 2018

Author Contributor

As you prefer. I followed the observation in the second comment there and the subsequent conversation, especially this comment.

Edit: IMHO, the refresh interval can't be negative.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 18, 2018

Member

I just prefer to avoid panic calls as much as possible.

This comment has been minimized.

@fengalin

fengalin Oct 18, 2018

Author Contributor

In that case, that should also be the behaviour for #251. My point is that according to the doc and the code I could read, only 0 is considered as "the value is not available". I agree a timestamp could be negative, but an interval which is computed from the succession of frame presentation times shouldn't.

@sdroege & @EPashkin, what do you think?

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 18, 2018

Member

Oh indeed, I didn't pay attention to that!

This comment has been minimized.

@sdroege

sdroege Oct 18, 2018

Member

IMHO this should panic as it's unexpected behaviour for the bindings and None has a different meaning. A negative value would have to be returned somehow if it is valid, but from our understanding it is never valid.

If this assumption is ever wrong, it has to fail hard and we have to fix it at the bindings level.

The way how the code is right now seems correct to me.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 18, 2018

Member

Uncatchable errors seems like a really bad issue for me. None and Err variants have been created for cases like this... We can always return None and use eprintln! to show that something problematic happened like GTK does.

This comment has been minimized.

@sdroege

sdroege Oct 18, 2018

Member

This is an uncatchable error by definition. It's like a function returning NULL when it really shouldn't and we don't expect it to either.

Making users suffer by returning an enum for all 3 cases does not seem like a good idea here. This case should just never happen at all, users are not supposed to handle it because it's a problem on our side and not on their side.

This comment has been minimized.

@fengalin

fengalin Oct 18, 2018

Author Contributor

I believe this is not an error in the sense "something the user should handle", but rather a bug either on the GTK side or due to our interpretation. Just like @sdroege said, if we were wrong, we should fix it. In that sense, the assertion violation message is a propos WRT this context.

My tests are far from being enough to serve as a proof that this can't happen, but at least it didn't show up with a couple of videos on a Linux host and a Windows host.

Still, I have no hard feelings with this and could just as well use a Result and eprintln! in case the value is negative. If you think that should be done, I'll also open another PR to fix #251.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 18, 2018

Member

No, you both convinced me. I'll go for this code then!

@GuillaumeGomez GuillaumeGomez merged commit 1843c8e into gtk-rs:master Oct 18, 2018

2 checks passed

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

@fengalin fengalin deleted the fengalin:refresh_interval_option branch Oct 18, 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.