wxGTK: Expose GtkTreeView drop position #2240

remyhorton · 2021-02-18T23:06:49Z

Within a wxDataViewCtrl drag-and-drop the underlying GtkTreeView
in wxGTK provides visual hints whether the drop is before/on/after
the destination row, but this information is not exposed to the
end developer. This proof-of-concept adds this functionality.

The main change is connecting to the drag-data-get and
drag-data-received signals which as far as I know were introduced
in GTK3, and are able to provide more information then
GtkTreeDragSourceIface and GtkTreeDragDestIface.

Within a wxDataViewCtrl drag-and-drop the underlying GtkTreeView in wxGTK provides visual hints whether the drop is before/on/after the destination row, but this information is not exposed to the end developer. This proof-of-concept adds this functionality. The main change is connecting to the drag-data-get and drag-data-received signals which as far as I know were introduced in GTK3, and are able to provide more information then GtkTreeDragSourceIface and GtkTreeDragDestIface.

vadz

Thanks, this would be definitely useful to have, but I think this should reuse the existing m_proposedDropIndex rather than adding another pair of accessors because it looks like the same thing to me, isn't it?

I.e. ideally we'd change wxGTK code so that the existing MyFrame::OnDrop() handler in samples/dataview/dataview.cpp would show the correct index when something is being dropped.

Could you please try to update the code to work like this? TIA!

src/gtk/dataview.cpp

vadz · 2021-02-20T12:36:11Z

src/gtk/dataview.cpp

@@ -4630,6 +4634,30 @@ gtk_dataview_motion_notify_callback( GtkWidget *WXUNUSED(widget),
    return FALSE;
 }

+static void evtDragDataGet(GtkWidget *widget, GdkDragContext *WXUNUSED(context),


Minor, but it would be nice to use the usual convention for the GTK callbacks names, i.e. wx_gtk_dataview_drag_data_get_callback for this one.

remyhorton · 2021-02-20T21:41:24Z

Looks like m_proposedDropIndex specifies what the (prospective) target row is rather than distinguishing between above/in/below said row. Having said that the associated accessor GetProposedDropIndex() is specified for wxEVT_DATAVIEW_ITEM_DROP_POSSIBLE and not wxEVT_DATAVIEW_ITEM_DROP so I see no issue in reusing it even if it is not quite the same thing..

vadz · 2021-02-20T23:07:56Z

Maybe I'm just misunderstanding what is this supposed to be used for, but how exactly is this different from the proposed drop index? I.e. how would you use this index other than for determining where to insert the new data?

remyhorton · 2021-02-21T02:29:46Z

Consider the four cases in composite screenshot:

GTK reports them as:

Path 1, GTK_TREE_VIEW_DROP_INTO_OR_AFTER
Path 1, GTK_TREE_VIEW_DROP_AFTER
Path 1:0, GTK_TREE_VIEW_DROP_BEFORE
Path 1:0, GTK_TREE_VIEW_DROP_INTO_OR_AFTER

How would these four map to m_proposedDropIndex values, since these are four distinct cases over only two rows..?

Maybe I should get to sleep now..

vadz · 2021-02-21T22:46:40Z

If the index of "Microcontrollers" is N, then the proposed drop index should be N in the first case and N+1 in all the other ones. In the first approximation, this could already be enough (and would definitely be better than nothing at all, as now), but if we need to distinguish between the other cases, we should also add BEFORE, AFTER and ONTO (I don't understand the GTK "into or after" name, "onto" seems more appropriate) flags to wx API too. And, ideally, implement them for the generic version too.

remyhorton · 2021-02-22T13:02:51Z

I think the idea behind labelling them INTO_OR_BEFORE & INTO_OR_AFTER is because GTK does not have an equivilent of IsContainer so it is up to the application to decide whether a dropped row is to be a child or sibling. It is not the only strange nomenclature GTK has..

Maybe n_dragFlags is the place to put them but will need to have a closer look at datavgen.cpp to make sure there's no conflict. Hopefully I can get another patch together before I fly over to Dublin.

remyhorton · 2021-03-01T13:35:47Z

Only build of generic wxDataViewCtrl that I could get working was wxMSW via MinGW/Wine so looks like there are issues with Bitrot. Seeing if I can replicate the same behaviour there that I've implemented with wxGTK...

New changes

vadz

Sorry, but unfortunately this still can't be applied because I don't think this provides a usable API for wx users (I also still have questions about the implementation, but this is secondary).

I.e. IMO the goal here should be to make the dataview sample work, i.e. determine the correct drop location, under all platforms. Right now this isn't the case because it doesn't work in wxGTK and this PR doesn't really help with it, as it just adds a different API which doesn't work in the other ports, while we really need something working in all of them.

src/gtk/dataview.cpp

vadz · 2021-04-09T13:39:47Z

include/wx/dnd.h

+// Flags for wxDataViewEvent::GetDragFlags() with wxEVT_DATAVIEW_DROP
+// Note: 0x05 & 0x06 are valid combinations!
+enum wxDropFlags


Naming-wise it's not great to use wxDropFlags as return value of GetDragFlags(), "drag" and "drop" are different verbs. It looks like GetDragFlags() is actually not used at all right now, which is, I guess, why you've decided to reuse it, but I'd still rather add GetDropFlags() if we want to return these flags.

But more importantly, I'd still like to somehow reuse GetProposedDropIndex() for this because otherwise you simply can't write portable code working correctly. I.e. you can either use the proposed drop index and it would work in the generic and Cocoa versions (i.e. under MSW and Mac) or use the drop flags and then it would work with wxGTK, but there is no way to make it work with both which is clearly not ideal.

Even if we can't represent all the possibilities using the proposed drop index, we should at least at least fill it when we can. I.e. it would be fine to provide some extra information in wxGTK which is not available under the other platforms, but we need to have at least some functionality common to all implementations.

My intention is to at least add the new flags to the generic dataview, but this looks like it will take a while to figure out and unfortunately other things have been making calls on my time. Might be able to do some trickery with GTK's map_expanded_rows in order to implement GetProposedDropIndex with relatively few lines of code, but not sure how closely it will emulate how generic does things.

Probably should tag this as WIP for the time being.

I think there is an inevitable API break here.
In cases where the item is dropped between two rows, wxGTK will select the lower row as what ends up returned by event.GetItem() whereas wxMSW (i.e. generic) will select the parent of the rows. Which one should be adopted as the universal convention?

I think there is an inevitable API break here.

This is not necessarily catastrophic (although best avoided, of course), the only "hard" requirement is to have at least a useful subset of the API actually working the same on all platforms, but I don't even think we need to break the API here.

In cases where the item is dropped between two rows, wxGTK will select the lower row as what ends up returned by event.GetItem() whereas wxMSW (i.e. generic) will select the parent of the rows. Which one should be adopted as the universal convention?

The latter behaviour seems more useful to me and this is also how the native macOS version behaves, so I'd strongly prefer to keep it like this and make wxGTK behave in the same way. Would it be possible to do it like this please? TIA!

I'll see what i can put together.

Did some quick testing with samples/dataview which looks like it was made for wxGeneric's reporting convention.

This is a reimplementation of how wxMSW cross-compiled on Linux using MinGW-w64 and run using Wine behaved so it is possible the behaviour is incorrect. Tested with a simple 3-deep tree that might have missed edge cases. What gets set for wxEVT_DATAVIEW_ITEM_DROP_POSSIBLE is not the same for wxMSW and wxGTK due to data not being available at the associated callback :( GTK2 vs. GTK3 vs. GTK4 also needs testing.

Some tidyup and testing still required. Need to change names to follow wxWidget coding standards. In wxGeneric event.GetDataBuffer() appears to be broken.

remyhorton added 3 commits February 18, 2021 22:58

Added WXUNUSED() directives

806b6a1

Added wxUSE_DRAG_AND_DROP conditionals to header

a0ff870

vadz added GTK work needed Too useful to close, but can't be applied in current state labels Feb 20, 2021

vadz previously requested changes Feb 20, 2021

View reviewed changes

remyhorton added 2 commits February 26, 2021 20:31

Reworked based on feedback

e4d7eff

Rogue comma removed

3cf8d10

vadz removed the work needed Too useful to close, but can't be applied in current state label Apr 7, 2021

vadz requested changes Apr 9, 2021

View reviewed changes

vadz added the work needed Too useful to close, but can't be applied in current state label Apr 9, 2021

remyhorton added 4 commits April 9, 2021 20:57

Add m_dropFlags and associated GetDropFlags()/GetDropFlags()

db29860

Made wxGTK follow wxGeneric's drag-drop model

9ca00d9

Some tidyup and testing still required. Need to change names to follow wxWidget coding standards. In wxGeneric event.GetDataBuffer() appears to be broken.

Minor tidyups

3fb574d

remyhorton requested a review from vadz April 13, 2021 22:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wxGTK: Expose GtkTreeView drop position #2240

wxGTK: Expose GtkTreeView drop position #2240

remyhorton commented Feb 18, 2021

vadz left a comment

vadz Feb 20, 2021

remyhorton commented Feb 20, 2021

vadz commented Feb 20, 2021

remyhorton commented Feb 21, 2021

vadz commented Feb 21, 2021

remyhorton commented Feb 22, 2021

remyhorton commented Mar 1, 2021

vadz left a comment

vadz Apr 9, 2021

remyhorton Apr 9, 2021

remyhorton Apr 9, 2021

vadz Apr 9, 2021

remyhorton Apr 10, 2021

remyhorton Apr 13, 2021

wxGTK: Expose GtkTreeView drop position #2240

Are you sure you want to change the base?

wxGTK: Expose GtkTreeView drop position #2240

Conversation

remyhorton commented Feb 18, 2021

vadz left a comment

Choose a reason for hiding this comment

vadz Feb 20, 2021

Choose a reason for hiding this comment

remyhorton commented Feb 20, 2021

vadz commented Feb 20, 2021

remyhorton commented Feb 21, 2021

vadz commented Feb 21, 2021

remyhorton commented Feb 22, 2021

remyhorton commented Mar 1, 2021

vadz left a comment

Choose a reason for hiding this comment

vadz Apr 9, 2021

Choose a reason for hiding this comment

remyhorton Apr 9, 2021

Choose a reason for hiding this comment

remyhorton Apr 9, 2021

Choose a reason for hiding this comment

vadz Apr 9, 2021

Choose a reason for hiding this comment

remyhorton Apr 10, 2021

Choose a reason for hiding this comment

remyhorton Apr 13, 2021

Choose a reason for hiding this comment