Added ability for escape key to close editing box in Value column in the Parameters panel

[nickleus27]

#2411

I added changes in cellrenderer_value.cpp. I added the on_key_press_event method to check for escape key and call on_editing_done(). I also had to add the signal_key_press_event() for the value widget.

@ice0 @rodolforg Hey guys can you check out these changes? Thanks again for all the advice to get me started.

[ice0]

@nickleus27
Works like a charm!
Ready to merge, but I want Rodolfo's review of this PR, maybe he has some ideas.

[nickleus27]

@ice0 Thank you. Sounds good.

[rodolforg]

It was a nice trick to simply prevent to call activate.

But somehow I think it should set property_editing_canceled to true before finish editing.
As I'm not sure about it, let it be :)

[nickleus27]

@rodolforg Yes, I think you are right. property_editing_canceled should be set to true. I will work on that and fix the change you requested.

[ice0]

It was a nice trick to simply prevent to call activate.

Where?

[nickleus27]

@rodolforg I am having some trouble getting bool property_editing_canceled to work. I am trying to connect on_editing_canceled() methods with signal_editing_canceled unsuccessfully. Both value_widget and value_entry say "no member named signal_editing_canceled" when I compile. Any advice?

[rodolforg]

It's a CellRenderer signal. So, CellRenderer_ValueBase should implement it.
In fact, now I see it even has a on_editing_canceled() as a default handler!

[nickleus27]

@rodolforg this is the error i am getting:
cellrenderer_value.cpp:699:16: error: no member named 'signal_editing_canceled' in 'studio::ValueBase_Entry' value_entry->signal_editing_canceled().connect(sigc::mem_fun(*this, &CellRenderer_ValueBase::on_value_editing_cancelled));

Not sure what I am doing wrong? ValueBase_Entry *value_entry; isn't that class a subclass of CellRenderer_ValueBase? Any help would be very much appreciated.

edit value_entry is a subclass of gtk::celleditable

[nickleus27]

@ice0 @rodolforg Hey fellows, can you check these changes out when you have a chance. There is a problem with these changes for sure. When you escape a value entry more than once it produces this output error:

synfig(11949) [12:22:36] error: on_value_editing_canceled(): Called twice!

[rodolforg]

Maybe we should leave as you did before XD
Simpler and worked. I'm unsure about this property behavior now :P
I'm really sorry.

[nickleus27]

@rodolforg No problem, I reverted back to previous solution. I enjoyed learning about on_editing_canceled(). I wish I could have gotten it to work properly. Nothing I tried seemed to work though.

[ice0]

Hi, guys!
I made a screenshot, just to show what is going on here.

First. Cell in Value row is CellRenderer_ValueBase : public Gtk::CellRendererText (docs)

It is not editable and is only used to display the cell value. But when you click on it, it emits a start_editing signal in which we can create a temporary editable cell (docs). This signal is not caught in Synfig code, instead of this overridden start_editing_vfunc is used here.

Next we creating editable cell ValueBase_Entry : public Gtk::CellEditable

synfig/synfig-studio/src/gui/cellrenderer/cellrenderer_value.cpp

Lines 655 to 659 in ec35bd8

	value_entry = manage(new ValueBase_Entry());
	value_entry->set_path(path);
	value_entry->set_canvas(get_canvas());
	value_entry->set_param_desc(get_param_desc());
	value_entry->set_value_desc(get_value_desc());

This class is the place where we can catch keypresses and other events and should signal to the parent cell when editing is done or canceled. If editing is canceled then we should set property_editing_canceled() to true, so parent can check this property and handle the event correctly (it should call stop_editing(true) method). After that we should emit signal_editing_done().

I think the current implementation works as expected only because it just hides the widget without emitting signals.
For example if you change on_editing_done() to editing_done() as it should be, then the value will be changed even then you hit the Esc key.

To make it work correctly you need to set property_editing_canceled() in ValueBase_Entry and check this property in CellRenderer_ValueBase::on_value_editing_done.

[nickleus27]

Thank you @ice0 ! I will work on this. :)

[nickleus27]

@ice0 Am I understanding this correctly? I should keep on_key_press_event() where it is in class ValueBase_Entry? I added property_editing_canceled() = true; to on_key_press_event() and I am checking for it in CellRenderer_ValueBase::on_value_editing_done. I am having trouble with the stop_editing(true) and emitting signal_editing_done correctly. Every way I have tried so far does not work as expected.

[ice0]

@ice0 Am I understanding this correctly? I should keep on_key_press_event() where it is in class ValueBase_Entry?...

@nickleus27
Yes, this is correct. Just push your code, so I can check it.

[ice0]

@ice0 my code doesn't seem to be calling on_value_editing_done() when you press esc? Not sure what is missing.

Yes. on_value_editing_done() is called when signal_editing_done() is emitted.

value_entry->signal_editing_done().connect(sigc::mem_fun(*this, &CellRenderer_ValueBase::on_value_editing_done));

But Esc key handler doesn't emit signal, it's just calls on_editing_done() method.

if(key_event->keyval == GDK_KEY_Escape)
{
	...
	on_editing_done();

To emit signal_editing_done() signal you should use editing_done() method here(docs).

[nickleus27]

@ice0 alright I think this is it. Thank you for being so patient. I had made this assumption that on_editing_done() should be emitting the signal_editing_done(), but that assumption was wrong. You had mentioned this a while ago, but I was reluctant to make sense of it.

[ice0]

@ice0 alright I think this is it. Thank you for being so patient.

You are welcome!

[nickleus27]

@ice0 Yes, good idea! I like the idea of returning from on_value_editing_done if property_editing_canceled is true. Much cleaner and simpler.

[rodolforg]

In case of @ice0 doesn't think my comment is relevant, I let here my approval :)

[rodolforg]

Very nice and clean :)

[ice0]

Merged. Thank you!

[nickleus27]

@ice0 thank you