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

Generate streams #69

Merged
merged 21 commits into from Dec 17, 2017

Conversation

Projects
None yet
4 participants
@EPashkin
Copy link
Member

EPashkin commented Dec 9, 2017

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 9, 2017

👍

Gir.toml Outdated
[[object.function]]
name = "read_line"
#return vec of char*
ignore = true

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

That should just return a Vec<u8> and I thought gir handles that by now (note that the element type is set to guint8 in the gir file, it's not utf8).

This comment has been minimized.

@EPashkin

EPashkin Dec 9, 2017

Author Member

Gir generated good looking code:

    fn read_line<'a, P: Into<Option<&'a Cancellable>>>(&self, cancellable: P) -> Result<(Vec<u8>, usize), Error> {
        let cancellable = cancellable.into();
        let cancellable = cancellable.to_glib_none();
        unsafe {
            let mut length = mem::uninitialized();
            let mut error = ptr::null_mut();
            let ret = ffi::g_data_input_stream_read_line(self.to_glib_none().0, &mut length, cancellable.0, &mut error);
            if error.is_null() { Ok((FromGlibPtrContainer::from_glib_full(ret), length)) } else { Err(from_glib_full(error)) }
        }
    }

but compiling get 2 errors:

error[E0277]: the trait bound `u8: glib::translate::Ptr` is not satisfied
   --> src\auto\data_input_stream.rs:145:38
    |
145 |             if error.is_null() { Ok((FromGlibPtrContainer::from_glib_full(ret), length)) } else { Err(from_glib_full(error)) }
    |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `glib::translate::Ptr` is not implemented for `u8`
    |
    = note: required because of the requirements on the impl of `glib::translate::FromGlibPtrContainer<u8, *mut u8>` for `std::vec::Vec<u8>`
    = note: required by `glib::translate::FromGlibPtrContainer::from_glib_full`

error[E0277]: the trait bound `u8: glib::translate::FromGlibPtrArrayContainerAsVec<u8, *mut u8>` is not satisfied
   --> src\auto\data_input_stream.rs:145:38
    |
145 |             if error.is_null() { Ok((FromGlibPtrContainer::from_glib_full(ret), length)) } else { Err(from_glib_full(error)) }
    |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `glib::translate::FromGlibPtrArrayContainerAsVec<u8,
 *mut u8>` is not implemented for `u8`
    |
    = note: required because of the requirements on the impl of `glib::translate::FromGlibPtrContainer<u8, *mut u8>` for `std::vec::Vec<u8>`
    = note: required by `glib::translate::FromGlibPtrContainer::from_glib_full`

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

Hmm, possibly a bug in glib/translate.rs. Exactly that impl should be used here and should be usable, unclear why it isn't. The glib::translate::Ptr requirement looks dubious.

Can you create a ticket?

This comment has been minimized.

@EPashkin
[[object.function]]
name = "splice_async"
#g_io_stream_splice_finish accept only 2 parameters instead normal 3 (no source_object)
ignore = true

This comment has been minimized.

@sdroege

This comment has been minimized.

@antoyo

antoyo Dec 9, 2017

Member

Do I need to update gir or something?


fn get_buffer_size(&self) -> usize;

fn peek(&self, buffer: &[u8], offset: usize) -> usize;

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

The buffer needs to be mutable, the function reads into this buffer

This comment has been minimized.

@EPashkin

EPashkin Dec 9, 2017

Author Member

Seems this function need be manual :(

}

impl<O: IsA<Converter>> ConverterExt for O {
fn convert(&self, inbuf: &[u8], outbuf: &[u8], flags: ConverterFlags) -> Result<(ConverterResult, usize, usize), Error> {

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

outbuf must be mutable

This comment has been minimized.

@EPashkin

EPashkin Dec 9, 2017

Author Member

Will made all Converter manual

// unsafe { TODO: call ffi::g_pollable_stream_read() }
//}
#[cfg(any(feature = "v2_34", feature = "dox"))]
pub fn pollable_stream_read<'a, P: IsA<InputStream>, Q: Into<Option<&'a Cancellable>>>(stream: &P, buffer: &[u8], blocking: bool, cancellable: Q) -> Result<isize, Error> {

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

This should not be in functions, but in InputStream (and the other one in OutputStream). Also calling this with blocking=false on a non-GPollableInputStream is undefined behaviour, thus unsafe. I would just not expose this at all, it's C convenience stuff with footguns

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

Also the buffer needs to be writable

This comment has been minimized.

@EPashkin

EPashkin Dec 9, 2017

Author Member

Ok I ignore this function.

This comment has been minimized.

@EPashkin

EPashkin Dec 9, 2017

Author Member

Maybe ignore pollable_stream_write too? PollableStreams not generated.

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

Yes, I meant both


fn is_closed(&self) -> bool;

fn read<'a, P: Into<Option<&'a Cancellable>>>(&self, buffer: &[u8], cancellable: P) -> Result<isize, Error>;

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

Buffer in all the read functions must be mutable

#[cfg(any(feature = "v2_44", feature = "dox"))]
fn read_all_async<'a, P: Into<Option<&'a Cancellable>>, Q: Fn(Result<usize, Error>) + Send + Sync + 'static>(&self, buffer: &[u8], io_priority: i32, cancellable: P, callback: Q);

fn read_async<'a, P: Into<Option<&'a Cancellable>>, Q: Fn(Result<(), Error>) + Send + Sync + 'static>(&self, buffer: &[u8], io_priority: i32, cancellable: P, callback: Q);

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

And this buffer must even stay alive until the async callback is called

This comment has been minimized.

@EPashkin

EPashkin Dec 9, 2017

Author Member

There way to do it?

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

With autogenerated code? No, not right now

This comment has been minimized.

@EPashkin

EPashkin Dec 9, 2017

Author Member

In manual code?

This comment has been minimized.

@sdroege
fn is_closing(&self) -> bool;

//#[cfg(any(feature = "v2_40", feature = "dox"))]
//fn printf<'a, P: Into<Option<&'a Cancellable>>>(&self, cancellable: P, error: &mut Error, format: &str, : /*Unknown conversion*//*Unimplemented*/Fundamental: VarArgs) -> Option<usize>;

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

Wat! I didn't know this function exists. Good to know :)

#[cfg(any(feature = "v2_44", feature = "dox"))]
fn write_all_async<'a, P: Into<Option<&'a Cancellable>>, Q: Fn(Result<usize, Error>) + Send + Sync + 'static>(&self, buffer: &[u8], io_priority: i32, cancellable: P, callback: Q);

fn write_async<'a, P: Into<Option<&'a Cancellable>>, Q: Fn(Result<(), Error>) + Send + Sync + 'static>(&self, buffer: &[u8], io_priority: i32, cancellable: P, callback: Q);

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

Buffer must stay alive until the async callback (or rather the finish method) is called

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 9, 2017

Mostly looks ok, just that all read functions need a mutable buffer, all read/write/etc async functions need their buffer to stay alive until the async callback is called, and various other minor things.

@EPashkin EPashkin force-pushed the EPashkin:streams branch from d54fe0b to e05c9e8 Dec 9, 2017

@EPashkin EPashkin referenced this pull request Dec 9, 2017

Merged

Regen #31

@EPashkin EPashkin force-pushed the EPashkin:streams branch from e05c9e8 to 53e33ce Dec 9, 2017

unsafe {
ffi::g_input_stream_read_bytes_async(self.to_glib_none().0, count, io_priority, cancellable.0, Some(callback), Box::into_raw(user_data) as *mut _);
}
}

This comment has been minimized.

@EPashkin

EPashkin Dec 9, 2017

Author Member

Seems this function generated wrong, see https://developer.gnome.org/gio/stable/GInputStream.html#g-input-stream-read-bytes-async
Don't understand how user get glib::Bytes from it
cc @antoyo

This comment has been minimized.

@sdroege

sdroege Dec 9, 2017

Member

g_input_stream_read_bytes_finish() gives you the GBytes

@EPashkin EPashkin force-pushed the EPashkin:streams branch from 53e33ce to eb08cb9 Dec 9, 2017

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 9, 2017

Made read_bytes_async manual,
There still question, how to "buffer must even stay alive until the async callback is called" even in manual code and this related to almost all async functions :(

@EPashkin EPashkin force-pushed the EPashkin:streams branch 2 times, most recently from 962b3e5 to 510f850 Dec 9, 2017

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 9, 2017

Make some async functions manual, moved entire file_input_stream.rs to manual as it have only two function and one of its query_info_async - wrong closure type.
Will add other changes as separate commits, to simplify checking.

@EPashkin EPashkin referenced this pull request Dec 9, 2017

Merged

Generate new types #604

@antoyo

This comment has been minimized.

Copy link
Member

antoyo commented Dec 9, 2017

@EPashkin So, if I understand correctly, what's missing is the return value (Ok value of the Result), right?

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 9, 2017

Yes.
In Result-returning functions we normally skip it only if return type void or bool

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 9, 2017

@sdroege All async IMHO "fixed"

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 9, 2017

But forgot make some buffers mutable, still fixing :(

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 9, 2017

Done too.
Only last problem is "buffer must even stay alive until the async callback is called" for all async operation.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 9, 2017

There still question, how to "buffer must even stay alive until the async callback is called" even in manual code and this related to almost all async functions :(

Look at the GStreamer Buffer function I linked elsewhere: https://github.com/sdroege/gstreamer-rs/blob/963557b79f5d3a91db944ed953a8c9fcab78a6ba/gstreamer/src/buffer.rs#L100

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 9, 2017

Sorry, I don't understand.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 9, 2017

You can take any AsMut<[u8]> here, but let's ignore that part for now and take Vec<u8>. You need to box it in the async function and then make that part of the user_data (https://github.com/sdroege/gstreamer-rs/blob/963557b79f5d3a91db944ed953a8c9fcab78a6ba/gstreamer/src/buffer.rs#L76-L81). In the finish function you would steal it from the user_data (i.e. move it out of there!), in the destroy_notify for the user_data you would drop it (like https://github.com/sdroege/gstreamer-rs/blob/963557b79f5d3a91db944ed953a8c9fcab78a6ba/gstreamer/src/buffer.rs#L67) if it was not stolen (see before).

@EPashkin EPashkin referenced this pull request Dec 9, 2017

Merged

New types #70

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 14, 2017

That was my point, you can actually use AsMut<[u8]> here and return the number of read bytes. It's basically the same as the Read trait. If that is considered safe, we can consider it safe here too.

@EPashkin EPashkin force-pushed the EPashkin:streams branch from ed0e2fc to 13ae905 Dec 14, 2017

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 14, 2017

Ok, I do this tomorrow, too late for today :(

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 14, 2017

Sure, no problem :)

@EPashkin EPashkin force-pushed the EPashkin:streams branch from 13ae905 to 5da2f24 Dec 15, 2017

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 15, 2017

Now src/input_stream.rs and src/output_stream.rs is done.
Others streams still not fully.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 15, 2017

👍

@EPashkin EPashkin force-pushed the EPashkin:streams branch from 5da2f24 to b837874 Dec 15, 2017

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 15, 2017

Seems Simplify test_util::run_async was bad idea

src/file.rs Outdated
use std::ptr;
use File;

pub trait FileExtManual {
fn append_to_async<'a, P: Into<Option<&'a Cancellable>>, Q: Fn(Result<FileOutputStream, Error>) + Send + Sync + 'static>(&self, flags: FileCreateFlags, io_priority: i32, cancellable: P, callback: Q);
fn append_to_async<'a, P: Into<Option<&'a Cancellable>>, Q: FnOnce(Result<FileOutputStream, Error>) + Send + 'static>(&self, flags: FileCreateFlags, io_priority: Priority, cancellable: P, callback: Q);

This comment has been minimized.

@sdroege

sdroege Dec 15, 2017

Member

Why are all these manual? Only because of the priority? I'd rather add support to gir for this instead, this is a huge amount of manual code otherwise :)

This comment has been minimized.

@EPashkin

EPashkin Dec 15, 2017

Author Member

I think that it part of gtk-rs/gir#504

This comment has been minimized.

@sdroege

sdroege Dec 15, 2017

Member

Not really, but it's a missing feature in gir indeed. It's a normal GIO-style async function not handled by gir yet

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 15, 2017

It because generated have Result<(),Error> instead needed

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 15, 2017

It because generated have Result<(),Error> instead needed

Oh why? g_file_append_to_finish() returns a FileOutputStream

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 15, 2017

@GuillaumeGomez , @sdroege
PR is ready for final review.

Gir.toml Outdated
@@ -43,7 +41,6 @@ generate = [
"Gio.SettingsBindFlags",
"Gio.SettingsSchema",
"Gio.SimpleActionGroup",
"Gio.SimpleIOStream",

This comment has been minimized.

@sdroege

sdroege Dec 15, 2017

Member

Why do you drop this one? It's not very important for bindings in any case

This comment has been minimized.

@EPashkin

EPashkin Dec 15, 2017

Author Member

It just moved as object to hide properties

@@ -31,8 +31,6 @@ pub trait FilterInputStreamExt {

fn set_close_base_stream(&self, close_base: bool);

fn connect_property_base_stream_notify<F: Fn(&Self) + 'static>(&self, f: F) -> SignalHandlerId;

This comment has been minimized.

@sdroege

sdroege Dec 15, 2017

Member

Maybe a gir configuration for this?

This comment has been minimized.

@sdroege

sdroege Dec 15, 2017

Member

I.e. generate = ["getter", "setter", "notify", "emit"]

This comment has been minimized.

@EPashkin

EPashkin Dec 15, 2017

Author Member

Good idea

This comment has been minimized.

@EPashkin

EPashkin Dec 15, 2017

Author Member

Added issue gtk-rs/gir#513

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 15, 2017

Seems good for me, I'm just worried by the huge amount of manual code and the inconsistencies around the io_priority stuff. Maybe let's get it in, and then make sure that gir gets fixed for handling most of these cases (i.e. all but the read/write functions)

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Dec 17, 2017

@GuillaumeGomez please, look at this PR too.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 17, 2017

Seems good to me as well, thanks!

@GuillaumeGomez GuillaumeGomez merged commit c004cb8 into gtk-rs:master Dec 17, 2017

2 checks passed

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

@EPashkin EPashkin deleted the EPashkin:streams branch Dec 17, 2017

vhdirk pushed a commit to vhdirk/gio-rs that referenced this pull request Jan 16, 2019

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.