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

Drag targets #472

Merged
merged 2 commits into from Apr 7, 2017

Conversation

Projects
None yet
3 participants
@Susurrus
Contributor

Susurrus commented Mar 17, 2017

Automatically generates TargetFlags and TargetList and manually implements TargetEntry. This follows the same pattern as that used by gdk::RGBA following @EPashkin's advice.

There are a few issues I see here:

  1. The in-memory layout of gdk::RGBA and gdk_sys::GdkRGBA is identical, so casting between them was trivial while I don't believe that holds true for gtk::TargetEntry and gtk_sys::GtkTargetEntry.
  2. I was getting compilation errors when autogenerating functions that tool an array of TargetEntrys so I disabled all functions that do that, though I obviously would like to have them as it includes the functions I want to use.
  3. I'm getting compilation errors for SetValue and SetValueOptional because of the datatype of to_glib_none(), which I didn't get with gdk::RGBA, but I don't know if I even need those traits implemented.

Closes #442.

Show outdated Hide outdated src/target_entry.rs Outdated
@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 17, 2017

Contributor

All of the fields for gdk::RGBA were public, but I don't think we want it for this type. After initialization i don't think these structs are really modified unlike with gdk::RGBA.

Contributor

Susurrus commented Mar 17, 2017

All of the fields for gdk::RGBA were public, but I don't think we want it for this type. After initialization i don't think these structs are really modified unlike with gdk::RGBA.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 17, 2017

Member

If it fields not public we need getters

Member

EPashkin commented Mar 17, 2017

If it fields not public we need getters

Show outdated Hide outdated src/target_entry.rs Outdated
Show outdated Hide outdated src/target_entry.rs Outdated
Show outdated Hide outdated src/target_entry.rs Outdated
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Mar 17, 2017

Member

I'd prefer getters then. It'll allow us to keep full control.

Member

GuillaumeGomez commented Mar 17, 2017

I'd prefer getters then. It'll allow us to keep full control.

Show outdated Hide outdated src/target_entry.rs Outdated
@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 17, 2017

Contributor

Should I also add 3 getters for all 3 fields (get_target, get_flags, and get_info)?

Contributor

Susurrus commented Mar 17, 2017

Should I also add 3 getters for all 3 fields (get_target, get_flags, and get_info)?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Mar 17, 2017

Member

Should I also add 3 getters for all 3 fields (get_target, get_flags, and get_info)?

Yes please.

Member

GuillaumeGomez commented Mar 17, 2017

Should I also add 3 getters for all 3 fields (get_target, get_flags, and get_info)?

Yes please.

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 21, 2017

Contributor

I think we're pretty close to a final solution here. I've removed the unnecessary glib::value trait impls, kept the field public in line with the other manually-implemented structs in Gtk, and fixed the ToGlibPtr* impls

Contributor

Susurrus commented Mar 21, 2017

I think we're pretty close to a final solution here. I've removed the unnecessary glib::value trait impls, kept the field public in line with the other manually-implemented structs in Gtk, and fixed the ToGlibPtr* impls

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 21, 2017

Contributor

Also there are several drag-related methods on Widgets that are blocking on the gdk::Atom in gtk-rs/gdk#145, so I suggest we wait to merge this until after that's merged.

Contributor

Susurrus commented Mar 21, 2017

Also there are several drag-related methods on Widgets that are blocking on the gdk::Atom in gtk-rs/gdk#145, so I suggest we wait to merge this until after that's merged.

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 21, 2017

Contributor

The last thing that needs to be done here is to handle the various drag functions that take an array of TargetEntrys, like WidgetExt::drag_dest_set (which is what #442 actually refers to). The hard part here is converting from a &[TargetEntry] to a &[ffi::GtkTargetEntry] to be passed as a pointer to ffi::gtk_drag_dest_set. What's the correct way to do this conversion as the following is not right:

    fn drag_dest_set(&self, flags: DestDefaults, targets: &[TargetEntry], actions: DragAction) {
        let glib_targets: Vec<ffi::GtkTargetEntry> = targets.iter().map(|e| e.to_glib_none().0).collect();
        unsafe {
            ffi::gtk_drag_dest_set(self.to_glib_none().0,
                                   flags.to_glib(),
                                   glib_targets.as_ptr() as *mut ffi::GtkTargetEntry,
                                   glib_targets.len() as c_int,
                                   actions.to_glib());
        }
    }
Contributor

Susurrus commented Mar 21, 2017

The last thing that needs to be done here is to handle the various drag functions that take an array of TargetEntrys, like WidgetExt::drag_dest_set (which is what #442 actually refers to). The hard part here is converting from a &[TargetEntry] to a &[ffi::GtkTargetEntry] to be passed as a pointer to ffi::gtk_drag_dest_set. What's the correct way to do this conversion as the following is not right:

    fn drag_dest_set(&self, flags: DestDefaults, targets: &[TargetEntry], actions: DragAction) {
        let glib_targets: Vec<ffi::GtkTargetEntry> = targets.iter().map(|e| e.to_glib_none().0).collect();
        unsafe {
            ffi::gtk_drag_dest_set(self.to_glib_none().0,
                                   flags.to_glib(),
                                   glib_targets.as_ptr() as *mut ffi::GtkTargetEntry,
                                   glib_targets.len() as c_int,
                                   actions.to_glib());
        }
    }
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 21, 2017

Member

See discussion from #472 (comment) (you first proposed that).

Member

EPashkin commented Mar 21, 2017

See discussion from #472 (comment) (you first proposed that).

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 21, 2017

Contributor

@EPashkin Indeed, and the limited examples I came across online didn't show any examples doing that. I've added another commit making them private and adding getters.

Contributor

Susurrus commented Mar 21, 2017

@EPashkin Indeed, and the limited examples I came across online didn't show any examples doing that. I've added another commit making them private and adding getters.

Show outdated Hide outdated src/target_entry.rs Outdated
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 21, 2017

Member

About drag_dest_set you can't collect e.to_glib_none().0 as this momently dropped.
I see this way: collect stashes

let stashes: Vec<_> = targets.iter().map(|e| e.to_glib_none()).collect();

create empty Vec and use set_len to allocate memory, then iterate stashes and copy field values for all GtkTargetEntrys.
May be there better way but I don't see it now.

Anyway this function need be tested that it really works.

Also all generated code say that it targets: &[&TargetEntry] not sure what really need (it both compiled with same code)

Member

EPashkin commented Mar 21, 2017

About drag_dest_set you can't collect e.to_glib_none().0 as this momently dropped.
I see this way: collect stashes

let stashes: Vec<_> = targets.iter().map(|e| e.to_glib_none()).collect();

create empty Vec and use set_len to allocate memory, then iterate stashes and copy field values for all GtkTargetEntrys.
May be there better way but I don't see it now.

Anyway this function need be tested that it really works.

Also all generated code say that it targets: &[&TargetEntry] not sure what really need (it both compiled with same code)

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 23, 2017

Contributor

I've done another pass, adding enough functionality here that we can actually implement a complete drag-and-drop example (I'm going to file a PR for the examples repo shortly with WIP examples) so this work can be tested in its entirety. I still don't have everything working, but I think this is pretty close. I think the gdk work with Atoms is the weak link right now.

This partially closes #475 and closes #474.

Contributor

Susurrus commented Mar 23, 2017

I've done another pass, adding enough functionality here that we can actually implement a complete drag-and-drop example (I'm going to file a PR for the examples repo shortly with WIP examples) so this work can be tested in its entirety. I still don't have everything working, but I think this is pretty close. I think the gdk work with Atoms is the weak link right now.

This partially closes #475 and closes #474.

Show outdated Hide outdated Gir.toml Outdated
Show outdated Hide outdated src/widget.rs Outdated
Show outdated Hide outdated src/widget.rs Outdated
@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 26, 2017

Contributor

I'm having trouble trying to use this code and I'm not certain where the error is. If I try and use the new Widget::drag_source_set() function like:

button.drag_source_set(gdk::MODIFIER_MASK,
                       &[],
                       gdk::ACTION_COPY);

I get datatype errors like:

error[E0308]: mismatched types
  --> src/drag_and_drop.rs:18:28
   |
18 |     button.drag_source_set(gdk::MODIFIER_MASK,
   |                            ^^^^^^^^^^^^^^^^^^ expected struct `gdk::auto::flags::ModifierType`, found struct `gdk::ModifierType`
   |
   = note: expected type `gdk::auto::flags::ModifierType`
              found type `gdk::ModifierType`

Anyone have any idea what this is actually saying? If I replace line 18 with gdk::auto::flags::ModifierType, I get auto is a private module. So I'm a little lost here.

Contributor

Susurrus commented Mar 26, 2017

I'm having trouble trying to use this code and I'm not certain where the error is. If I try and use the new Widget::drag_source_set() function like:

button.drag_source_set(gdk::MODIFIER_MASK,
                       &[],
                       gdk::ACTION_COPY);

I get datatype errors like:

error[E0308]: mismatched types
  --> src/drag_and_drop.rs:18:28
   |
18 |     button.drag_source_set(gdk::MODIFIER_MASK,
   |                            ^^^^^^^^^^^^^^^^^^ expected struct `gdk::auto::flags::ModifierType`, found struct `gdk::ModifierType`
   |
   = note: expected type `gdk::auto::flags::ModifierType`
              found type `gdk::ModifierType`

Anyone have any idea what this is actually saying? If I replace line 18 with gdk::auto::flags::ModifierType, I get auto is a private module. So I'm a little lost here.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 26, 2017

Member

It again old manual duplicate https://github.com/gtk-rs/gdk/blob/master/src/enums.rs#L5-L27 that we forgot to remove

Member

EPashkin commented Mar 26, 2017

It again old manual duplicate https://github.com/gtk-rs/gdk/blob/master/src/enums.rs#L5-L27 that we forgot to remove

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 26, 2017

Member

No, it something else

Member

EPashkin commented Mar 26, 2017

No, it something else

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 26, 2017

Contributor

Regarding your point of enums, it does seem odd to me that there are two declarations of MODIFIER_MASK:

src/auto/mod.rs:pub use self::flags::MODIFIER_MASK;
src/enums.rs:    pub const ModifierMask: ffi::GdkModifierType = ffi::GDK_MODIFIER_MASK;

For my code above should I have used gdk::enums::modifier_type::ModifierMask?

Contributor

Susurrus commented Mar 26, 2017

Regarding your point of enums, it does seem odd to me that there are two declarations of MODIFIER_MASK:

src/auto/mod.rs:pub use self::flags::MODIFIER_MASK;
src/enums.rs:    pub const ModifierMask: ffi::GdkModifierType = ffi::GDK_MODIFIER_MASK;

For my code above should I have used gdk::enums::modifier_type::ModifierMask?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 27, 2017

Member

@Susurrus No gdk::enums::modifier_type must be not used.
Strange, I checked your code on examples/basic and compiles it both nightly and stable

Member

EPashkin commented Mar 27, 2017

@Susurrus No gdk::enums::modifier_type must be not used.
Strange, I checked your code on examples/basic and compiles it both nightly and stable

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 27, 2017

Contributor

@EPashkin Yeah, looks like I had a bad path in my Cargo.toml. Alright, I'll get back to it, thanks!

Contributor

Susurrus commented Mar 27, 2017

@EPashkin Yeah, looks like I had a bad path in my Cargo.toml. Alright, I'll get back to it, thanks!

Show outdated Hide outdated src/widget.rs Outdated
Show outdated Hide outdated src/widget.rs Outdated
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 30, 2017

Member

IMHO SelectionData::set_text etc. don't need &mut self. While it still don't generate received signal, text changed.

    [[object.function]]
    name = "set_text"
        [[object.function.parameter]]
        name = "selection_data"
        const = true

Also Widget::drag-data-get signal don't need be mut and manual

Member

EPashkin commented Mar 30, 2017

IMHO SelectionData::set_text etc. don't need &mut self. While it still don't generate received signal, text changed.

    [[object.function]]
    name = "set_text"
        [[object.function.parameter]]
        name = "selection_data"
        const = true

Also Widget::drag-data-get signal don't need be mut and manual

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 30, 2017

Contributor

Looking over the drag signals, I also think Widget::drag-drop needs to be set to inhibit = true, is that correct? I think it's for signals that return gboolean values, yes?

Contributor

Susurrus commented Mar 30, 2017

Looking over the drag signals, I also think Widget::drag-drop needs to be set to inhibit = true, is that correct? I think it's for signals that return gboolean values, yes?

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 30, 2017

Contributor

Also Widget::drag-motion I believe

Contributor

Susurrus commented Mar 30, 2017

Also Widget::drag-motion I believe

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 30, 2017

Member

You right for both

Member

EPashkin commented Mar 30, 2017

You right for both

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 30, 2017

Contributor

Alright, I've updated the code as you've suggested and set those 2 signals to inhibit. Hopefully this is in line with your comments.

Contributor

Susurrus commented Mar 30, 2017

Alright, I've updated the code as you've suggested and set those 2 signals to inhibit. Hopefully this is in line with your comments.

@@ -93,9 +93,9 @@ impl SelectionData {
}
}
pub fn set_text(&mut self, str: &str, len: i32) -> bool {
pub fn set_text(&self, str: &str, len: i32) -> bool {

This comment has been minimized.

@EPashkin

EPashkin Mar 30, 2017

Member

All others set need const too.

@EPashkin

EPashkin Mar 30, 2017

Member

All others set need const too.

Show outdated Hide outdated src/target_list.rs Outdated
@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Apr 7, 2017

Contributor

Here's an updated version that addresses all of your points.

Contributor

Susurrus commented Apr 7, 2017

Here's an updated version that addresses all of your points.

Show outdated Hide outdated src/widget.rs Outdated
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 7, 2017

Member

@Susurrus, rebase this to last master please
I don't see problem with code but way to use D&D still not found and this in WIP state?

Member

EPashkin commented Apr 7, 2017

@Susurrus, rebase this to last master please
I don't see problem with code but way to use D&D still not found and this in WIP state?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 7, 2017

Member

Seems I found problem: from_glib_none for boxed types use copy and you don't changed original.
So need use from_glib_borrow on data

    [[object.signal]]
    name = "drag-data-get"
        [[object.signal.parameter]]
        name = "data"
        transformation = "borrow"

By way, drag_and_drop_textview.rs example panic on windows as it produced wrong path
filename: /D:/eap/rust/0/0.txt

Member

EPashkin commented Apr 7, 2017

Seems I found problem: from_glib_none for boxed types use copy and you don't changed original.
So need use from_glib_borrow on data

    [[object.signal]]
    name = "drag-data-get"
        [[object.signal.parameter]]
        name = "data"
        transformation = "borrow"

By way, drag_and_drop_textview.rs example panic on windows as it produced wrong path
filename: /D:/eap/rust/0/0.txt

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 7, 2017

Member

From travis error this need skip_assert_initialized!();

$ ./check_init_asserts
src/target_list.rs: new(targets: &[TargetEntry]) -> Self 
Member

EPashkin commented Apr 7, 2017

From travis error this need skip_assert_initialized!();

$ ./check_init_asserts
src/target_list.rs: new(targets: &[TargetEntry]) -> Self 
@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Apr 7, 2017

Contributor

I think you did it @EPashkin! I've pushed the latest version which makes the examples work for me now. I think this does it!

Contributor

Susurrus commented Apr 7, 2017

I think you did it @EPashkin! I've pushed the latest version which makes the examples work for me now. I think this does it!

@Susurrus Susurrus changed the title from RFC: Drag targets to Drag targets Apr 7, 2017

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 7, 2017

Member

👍 Thanks, @Susurrus

Member

EPashkin commented Apr 7, 2017

👍 Thanks, @Susurrus

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 7, 2017

Member

Awesome! Thanks @Susurrus!

Member

GuillaumeGomez commented Apr 7, 2017

Awesome! Thanks @Susurrus!

@GuillaumeGomez GuillaumeGomez merged commit b75ced7 into gtk-rs:master Apr 7, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Susurrus Susurrus deleted the Susurrus:drag_targets branch Apr 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment