Fix reset of int options on option destructor #85
Conversation
There was a problem hiding this comment.
Seems like the bug affects double options as well, so the spinbox connection for OPTION_TYPE_DOUBLE should be stored and disconnected as well.
Tbh, I really hope there is a better solution which doesn't require storing and disconnecting those connections manually.
src/wcm.hpp
Outdated
| @@ -138,6 +136,8 @@ class OptionWidget : public Gtk::Box | |||
| Gtk::Label name_label; | |||
| std::vector<std::unique_ptr<Gtk::Widget>> widgets; | |||
| Gtk::Button reset_button; | |||
| sigc::connection int_spinbutton_connection; | |||
There was a problem hiding this comment.
I'd suggest using Nvm, it's a sigc::scoped_connection here, then no custom ~OptionWidget is necessary.sigc++-3.0 feature, and gtkmm-3.0 uses sigc++-2.0.
Also I wonder why don't use a single spinbutton_connection for both animation/int.
src/wcm.hpp
Outdated
| @@ -260,7 +261,7 @@ class OptionSubgroupWidget : public Gtk::Frame | |||
| { | |||
| Gtk::Expander expander; | |||
| Gtk::Box expander_layout = Gtk::Box(Gtk::ORIENTATION_VERTICAL, 10); | |||
| std::vector<OptionWidget> option_widgets; | |||
| std::vector<std::unique_ptr<OptionWidget>> option_widgets; | |||
There was a problem hiding this comment.
I wonder why is std::unique_ptr necessary (if it is).
There was a problem hiding this comment.
Yes it is necessary because a sigc::connection can be copied but not moved, whereas some other members can only be moved but not copied. So altogether you can neither copy nor move OptionWidget with this change.
|
I looked at the old code and found out that it would save the value on |
Is this a possible solution? Feel free to open another PR that replaces this one, or add commits to this branch if you can. Naturally, I'd like to fix the double options too, but I agree that the current solution is 'meh'. |
I'll try it today |
What about having the spinbuttons as part of the OptionWidget class, so (hopefully) they will be destroyed with the OptionWidget, and we wouldn't even have to store connections at all? EDIT: Something like this patch. |
The spinbuttons are stored in |
I see. Upon further inspection, my patch simply causes a segfault before it has a chance to for the bug to happen, so I'm currently back to square one on this, which is storing the connection handles.. |
|
Seems we are fighting a gtkmm bug here https://gitlab.gnome.org/GNOME/gtkmm/-/issues/128 |
Int durations were set to 0 after adjusting an int or animation option and susequently clicking the back button or closing wcm. Fix this by using the solution found in gtkmm bug #128 on gitlab. Thanks to ammen99 for pointing out the initial solution.
|
@NamorNiradnug There was a solution posted on the gtkmm bug thread, so I implemented it and it seems to be working. Are you able to double check and review please? |
LGTM, I'll test it a little later. |
|
I tested it and it seems to be working! |
That's good enough of a review for me! |
Int durations were set to 0 after adjusting an
int or animation option and susequently clicking
the back button or closing wcm. Fix this by
removing the changed signal handler on option
destructor. Thanks to ammen99 for the initial
idea and patch.