feat(studio): added buttons to change interpolation for selected waypoints

[mohamedAdhamc]

Added the five buttons asked for in #1267 .

Yet to do
1- add the separator
2- add the button functionality

update:
p.s. I believe these buttons should be normal gtk buttons not radio buttons but I will leave this for after I'm done with correctly applying the functionality to the buttons.

[mohamedAdhamc]

Hey guys @ice0 @Keyikedalube and anyone who could help xd,

I'm not even sure if the signal is connected properly but I came across something I find quite weird.
the CanvasView member function canvas_interface() is used without an object many times in canvasview.cpp
an example is in the on_waypoint_clicked_canvasview function
item->signal_activate().connect(/*connecting the signal*/\ sigc::bind(sigc::ptr_fun(set_waypoint_model), waypoint_set, model, canvas_interface()));
and everything works correctly
and when I do something very similar in another file as I did here
tcb_button->signal_toggled().connect(sigc::bind(sigc::ptr_fun(set_waypoint_model), waypoint_set_g, model, CanvasView::canvas_interface()))
it surely complains and says member function canvas_interface() cant be called without an object. which makes sense, what doesn't is the former case.

p.s
am I getting too carried away here ?

[Keyikedalube]

CanvasView member function canvas_interface() is used without an object many times in canvasview.cpp

Didn't really get what you meant by that but I'm hoping you mean CanvasInterface object itself

CanvasInterface object gets instantiated in CanvasView constructor initializer lists in line 517

synfig/synfig-studio/src/gui/canvasview.cpp

Lines 511 to 517 in 08202f4

	CanvasView::CanvasView(etl::loose_handle<Instance> instance,etl::handle<CanvasInterface> canvas_interface_):
	Dockable(synfig::GUID().get_string(),_("Canvas View")),
	work_area (),
	activation_index_ (true),
	smach_ (this),
	instance_ (instance),
	canvas_interface_ (canvas_interface_),

In your code I believe CanvasView object is non-existent excepting these two protected member functions init_canvas_view_vfunc(...) and changed_canvas_view_vfunc(...) where the said object exists for a short time (locally). Hence, your call to this CanvasView::canvas_interface() fails because the object for CanvasView is not instantiated at all so you can't really access its member function canvas_interface()

I tried adding a new private member etl::loose_handle<CanvasView> canvas_view and assigned that when either init or changed canvas view vfunc was called. That way we actually have a canvas_view object in your modified Dock_Timtrack2 class. Changed your signal call to

if ( tcb_button->signal_toggled().connect(sigc::bind(sigc::ptr_fun(set_waypoint_model), waypoint_set_g, model, canvas_view->canvas_interface())) )
...

Code compiles successfully this time but the application crashes on launch.

I don't understand why you have a global function set_waypoint_model(...) on CanvasView class but that's not my concern LOL.

I hope my small contribution helps.

[mohamedAdhamc]

Code compiles successfully this time but the application crashes on launch.

yes I also did that and the application crashed before it started with a segmentation error xd. I think it may have to do with the return of canvas_interface() which is canvas_interface_ being a private member of the CanvasView class although I am not at all sure if that could be the cause of this segmentation error.

In your code I believe CanvasView object is non-existent excepting these two protected member functions init_canvas_view_vfunc(...) and changed_canvas_view_vfunc(...) where the said object exists for a short time (locally). Hence, your call to this CanvasView::canvas_interface() fails because the object for CanvasView is not instantiated at all so you can't really access its member function canvas_interface()

yes I feel like that makes much more sense now.

I don't understand why you have a global function set_waypoint_model(...) on CanvasView class but that's not my concern LOL.

I just did that so I can use it in the signal call for now and see what would happen, but it seems like something is really off. I'll have to look into it more.

[mohamedAdhamc]

update: realized what was done wrong, we could just use get_canvas_interface() and it works fine for getting the canvas_interface as its name implies xd.

now when the button is pressed the selected waypoint changes its interpolation type successfully. Only did it for two buttons as of now since the implementation is very basic however if this model is to be gone through instead of creating an action, I will make the lambda a separate function with a fourth argument being the interpolation type and just connect it to each buttons signal.

tasks left to do
1- change interpolation type for multiple widgets if multiple widgets are selected: I'll probably make an action in a similar way to the one for moving waypoints.

2- switch over the toggle buttons into regular gtk buttons as it makes more sense considering the functionality.

3- clean up the code. Its huge as of right now because I duplicated the lambda twice xd.

will be working on it hopefully tomorrow and if things go well maybe it could be completely done in about 2 days.

update: got caught up with sudden school work and now we have a holiday so the "2 days" was delayed a bit unfortuanetly xd. Im continuing work on it now and trying to understand the relation between Waypoint Items and waypoint_set , as the former is used in the processes of selecting multiople waypoints while the latter is the one used for setting interpolation type and other waypoint model things.

[mohamedAdhamc]

so as far as I see here the WaypointItem is a struct which is used along with WaypointSD to assist in dragging multiple waypoints however I couldnt draw a connection between it and the "waypoint_set" which is of a different -synfig::waypoint- type. I guess I could implement the select drag helper somehow and just set everything up for being able to capture the selection of waypoint_sets and maybe put them in a synfig::WaypointList somehow or just a regular vector. However, things are a bit messy right now trying to figure out all these things so in the mean time while im working on it. @ice0 and @Keyikedalube what do you guys think ? is this the way to go for this or is there something I'm missing here ?

just to summarize so far I have been able to set the waypoint interpolation type for just one waypoint which is clicked
now I'm just trying to set the interpolation type for multiple selected waypoints.

[Keyikedalube]

@mohamedAdhamc I'm sorry I can't help you here because I haven't explored these core parts of Synfig. I only contribute to the frontend ie., GUI, not the backend. @ice0 and @rodolforg are your best guides here.

BTW, hooray to you on your successful implementation of single-waypoints interpolation change!

[ice0]

2- switch over the toggle buttons into regular gtk buttons as it makes more sense considering the functionality.

Maybe it's better to use Gtk::RadioButton for this?
P.S. I haven't checked if they can be used with icons only.

[mohamedAdhamc]

2- switch over the toggle buttons into regular gtk buttons as it makes more sense considering the functionality.

Maybe it's better to use Gtk::RadioButton for this? P.S. I haven't checked if they can be used with icons only.

Well part of the reason I felt like maybe the regular buttons would be good for this is because one would just have to click on a single waypoint/select waypoints then hit the button which sets the needed interpolation type and thats it. However, with the check button it may get a bit more complicated due to a few points
1- size : Im also not sure of they can somehow be only used with icons, didnt even know they could be used with icons xd, however if not. Radio buttons would probably too big to fit into the tool panel. would probably have to add to a sepearate perhaps popup tool bar
2- there would have to be processing done on selection of waypoints to have the "tiny mark looking thing" in the proper interpolation type button upon selection of waypoints to display original type before the users sets the new one. Which also poses another question, on selection of multiple one of different type what should the behavior be ?

However, I believe this could probably be debated both ways and the button type wouldn't be really a problem to implement either way. What really needs a lot of work now is getting this to work for multiple waypoints at once 😄.

[mohamedAdhamc]

@mohamedAdhamc I'm sorry I can't help you here because I haven't explored these core parts of Synfig. I only contribute to the frontend ie., GUI, not the backend. @ice0 and @rodolforg are your best guides here.

Yes, no problem😄. Actually, this is part of the reason i'm having a bit more trouble here, it's probably one of the first times i come into such direct contact with the core xd.

BTW, hooray to you on your successful implementation of single-waypoints interpolation change!

Thanks! lets hope the whole feature would be finished soon.

[mohamedAdhamc]

update: looks like implementing a struct that inherits from select drag helper in order to handle waypoint_set selection is the way to go. then we can just store the selected items in a vector and iterate through it doing what was already implemented for a single waypoint to the multiple stored waypoints.

I'm currently starting its implementation, looks like it might take a bit but lets hope for the best, I believe this is a pretty cool feature so its worth it xD.

[mohamedAdhamc]

Got delayed a lot unfortuanetly due to college work which keeps on increasing and never decreases lol. however I was able to do something.

ok, so I used a diffrent approach. Just getting the selected waypoint items and storing them in a vector. then iterating through the vector and setting the waypoints. It worked and changes the selected waypoints interpolation. I put the code in "on_waypoint_clicked" just to test out if the concept would actually work.

so I guess now I would just have to add a new action which is set when the button is pressed and which calls a new function which I would make with a block of code very similar to the one i tried out here.

p.s forgot to clear the vector each time process starts over in the patch pushed, however the patch is just an update so its ok will have it in the next one.

Update: done it using setter function.

[mohamedAdhamc]

Alright, finally now I tested it and the functionality works fine. What's left would be just to switch over the radio buttons into regular buttons, and add the separator. Will be working on those in a couple hours, and hopefully then this would be mostly done except for having to be reviewed and possible optimizations suggested.

p.s. the additions are all over the place so its a bit hectic xD i keep adding the patches then going back to find I forgot something.

[mohamedAdhamc]

Hey guys,
@ice0 , @Keyikedalube , finally this feature is now 99% done, kept getting delayed due to college work xd, however thankfully found just about enough time.

Now one can click on the waypoint then the corresponding interpolation button to set the interpolation. or select multiple waypoints and do the same to set their interpolation. here is a link to a gif of the buttons
https://media.giphy.com/media/iunOh2awSEJ7kJejKN/giphy.gif

What's left is just to add the separator which for some reason isnt being added correctly. Also the buttons tool tip which I just realized I forgot to add.

Excited to hear your reviews guys xd.

p.s.
this is whats happening with the separator. I will paste the question i asked in the gnome forums.

So I know that tool palettes cant have widgets added to them directly, only tool item groups. I have a group with all the buttons i want and its added and everythings good. then I wanted to add a separator, and i did the regular way, here is a snippet of the code.

Gtk::SeparatorToolItem *seperator = Gtk::manage(new Gtk::SeparatorToolItem());
   seperator->show();
   tool_item_group->insert(*seperator);

but the problem here is that the separator ends up looking like this.

also it runs with the error.
Gtk-CRITICAL **: 03:20:29.865: gtk_widget_set_focus_on_click: assertion 'GTK_IS_WIDGET (widget)' failed

[mohamedAdhamc]

@rodolforg done, should I remove the separator ?

[ice0]

It's a Gtk theme issue. I think we should not use the separator :/

Where did you find this information?

As for me it looks like this feature just not implemented for Gtk::ToolPalette widget.

These are images of how the Gtk developers think it should be used. And I don't see any separators here.
Since the ToolPalette is a grid, it is not clear what to do if a separator is added. Should you split it horizontally or vertically?
How should you position the buttons?

Moreover, in the gtk tests, the separator is only tested with the toolbar.

https://github.com/GNOME/gtk/blob/51e636be0f1f154a3a007892401fc51104f88ca1/tests/testtoolbar.c#L609-L611

https://lazka.github.io/pgi-docs/Gtk-3.0/classes/ToolPalette.html
https://developer-old.gnome.org/gtkmm-tutorial/3.24/toolpalette-example.html.en

After switching from Gtk::ToolPalette to Gtk::Toolbar all works without issues.

@mohamedAdhamc
I suggest making these buttons disabled if there are no waypoints selected and add tooltips to them.

[mohamedAdhamc]

@mohamedAdhamc I suggest making these buttons disabled if there are no waypoints selected and add tooltips to them.

while switching over to `Gtk::ToolPalette' or keeping the current 'Gtk::Toolbar' ? I'm assuming it would be the latter, but I'm just making sure here.

[ice0]

while switching over to `Gtk::ToolPalette' or keeping the current 'Gtk::Toolbar' ? I'm assuming it would be the latter, but I'm just making sure here.

I suggest to switch to Gtk::Toolbar from Gtk::ToolPalette

[rodolforg]

Where did you find this information?

https://docs.gtk.org/gtk3/class.SeparatorToolItem.html

Depending on the theme, a GtkSeparatorToolItem will often look like a vertical line on horizontally docked toolbars.

[mohamedAdhamc]

@ice0 done.

[mohamedAdhamc]

@rodolforg done :)

[mohamedAdhamc]

@rodolforg done. Anything else needed here ?

[ice0]

You can use multiple calls to set_param("waypoint") to add multiple waypoints at the same time.
This will reduce the size of the history.

[mohamedAdhamc]

hmm, I'm not really ssure what you mean here. I thought maybe you mean to set the waypoint parameter for multiple waypoints then perform the action once, but it gives out errors (also I think setting the parameter "waypoint" multiple time before perform action willl just overwrite it so its probably not what you meant)

[ice0]

hmm, I'm not really ssure what you mean here. I thought maybe you mean to set the waypoint parameter for multiple waypoints then perform the action once, but it gives out errors

Yes.

(also I think setting the parameter "waypoint" multiple time before perform action will just overwrite it so its probably not what you meant)

You can check WaypointSet action to be sure it doesn't - it adds it to the vector.

The only issue I see here, is the value_node parameter. I don't know why it is passed, if it can be resolved from waypoint parameter. Also value_node overwrites previous value, so you need to check if:

1. it is used
2. it can't be resolved from waypoint parameter.

[mohamedAdhamc]

You can check WaypointSet action to be sure it doesn't - it adds it to the vector.

oh yes I checked it does add them to a vector but what is puzzling me is the value node, it is also what is causing the error when trying to perform the action for all the selected waypoints at once, the value node isn't set for each one of them. In fact I'm confused how it could be set for each one of them when there is only one value node handle in the WayPointSet ... maybe we need a value node vector similar to the waypoint vector or maybe there is one i'm not sure i will try to investigate here a bit.

The only issue I see here, is the value_node parameter. I don't know why it is passed, if it can be resolved from waypoint parameter. Also value_node overwrites previous value, so you need to check if:

1. it is used
2. it can't be resolved from waypoint parameter.

1- I believe yes it is used as it seems for the action WaypointSet to be performed it needs the waypoint and the valuenode of the waypoint, failure to have the valuenode gives out the error unable to find waypoint

2- i'm not sure what you mean exactly by resolve, however the valuenode of the waypoint can be gotten from the waypoint itself using for example get_parent_value_node, im thinking maybe i can add an extra default parameter for doing waypoint related actions like this without having to set the valuenode explicitly for each one, to make it implicit by making the valuenode be set from WaypointSet itself and stored in a vector. But I have to first understand a bit more of whats going on, so I will have to investigate further.

[mohamedAdhamc]

@ice0 , @rodolforg would this change be necessary here though, as in this PR. from what I see currently a similar situation which is setting interpolation from right mouse click groups the set waypoint actions into a passive group similar to what is done here, however to instead set the action once then waypointset class would have to be modified somewhat significantly.

p.s. I have managed to allow this by simply having a vector of valuenodeds in the action similar to the vector of waypoints but it seems like it could easily break something.

[ice0]

would this change be necessary here though

Ok, this can be fixed later.

Done

[ice0]

@morevnaproject Can you test it?

[ice0]

Merged. Thank you!