GNOME Bugzilla – Bug 346683
New "Rect select" tool doesn't restore settings
Last modified: 2006-11-05 15:11:46 UTC
Please describe the problem: No matter what I try, the new selection tool doesn't save it's state and settings as the old one from 2.2 did. I always have to reset every single setting to the values I want to have in there. Steps to reproduce: 1. Select the rectangular selection tool. 2. Open the "Rectangle Controls" 3. check Aspect ratio and enter 0.6666 next to it 4. Try to save the settings in any way possible (preferences dialog, save buttons on the tool control itself) 5. quit the GIMP 6. start the GIMP 7. select "Rect select" and the options are all reset to the initial defaults again Actual results: Expected results: I'd expect the tool to have the previously saved settings restored Does this happen every time? Yes Other information:
The properties registered by the GimpRectangleOptions interface need to have the GIMP_CONFIG_PARAM_SERIALIZE flag set. Otherwise they aren't saved/restored.
Shouldn't the GimpRectangleOptions interface properties be set with one of the macros out of the set GIMP_CONFIG_INSTALL_PROP_* instead of calling g_object_interface_install_property directly?
Have a look at the macros, that should answer your question. Also, it really doesn't make sense to discuss such details here. I only added the comment as a hint for the developers working on the new tools.
raising severity to blocker because it is necessary to fix this for 2.4.
Created attachment 70669 [details] [review] Patch to add serialize and construct flags This patch together with the work done by this checkin: 2006-08-08 Sven Neumann <sven@gimp.org> * app/tools/gimprectangleoptions.c (gimp_rectangle_options_interface_get_type): made GimpToolOptions a prerequisite of the GimpRectangleOptions interface. * app/tools/gimprectangletool.c (gimp_rectangle_tool_interface_get_type): made GimpDrawTool a prerequisite of the GimpRectangleTool interface. seems to fix the saving/restore issue. I have replaced the GIMP_PARAM_READWRITE with the GIMP_CONFIG_PARAM_FLAGS where apropiate, this flag combines G_PARAM_READWRITE, G_PARAM_CONSTRUCT and GIMP_PARAM_SERIALIZE and is the one used by the convenience macros used by the tool objects. Since the above change these flags seem apropiate in the interface as well... The remaining issue I have with the attached patch is that the Highlight-checkbox setting isn't restored.
Looks perfect except it should be GIMP_CONFIG_PARAM_FLAGS | GIMP_PARAM_STATIC_STRINGS so we don't duplicate all strings of the param specs.
I don't think that all properties should be serialized. It doesn't make sense to store selection size across sessions. Also some properties not even make sense as properties, they should simply be removed (new-fixed-width and new-fixed-height). It's sufficient to have them as fields in the options struct.
Thanks to Karl for his patch, I'll apply it (with Michael's modification suggestion) as soon as I am done with the properties cleanup.
I'm gone for another week, didn't clean the properties yet so I applied the patch - let's keep this bug open to remember about the cleanup. 2006-08-13 Karine Delvare <edhel@gimp.org> * app/tools/gimprectangleoptions.c: restore rectangle settings. Fixes bug #346683.
Created attachment 71334 [details] [review] Do not serialize everything Current status: Properties serialized * highlight * guide * new-fixed-width * new-fixed-height * fixed-aspect : these 3 work together. Sven, don't you think people will want to keep the fixed-width setting? * aspect-square * fixed-center * controls-expanded Properties not serialized * width * height * aspect : this one may change when the aspect setting gets better * center-x * center-y * unit * dimensions-entry I tried to get rid of some properties, but we need to keep new-fixed-width if we want to serialize it. I thought I could remove unit, but gimp_prop_size_entry_new needs a property AND a unit property... Anything I missed?
GIMP_PARAM_STATIC_STRINGS can stay, it doesn't have anything to do with serialization. Apart from that, the patch looks good for a start. I would suggest that you get rid of the dimensions-entry property altogether. The code for this should be moved into the tool anyway, it doesn't belong to the tool options.
Please use Milestone + Priority==Urgent instead of Severity==Blocker.
Created attachment 71487 [details] [review] Properties cleanup Work in progress, I still get an "assertion `GIMP_IS_SIZE_ENTRY (gse)' failed". If anyone wants to review this, I'd be glad to know if I'm going the right way.
There is still property code for the "dimensions-entry" property that is supposed to be removed. And, as far as I can see, noone ever calls gimp_rectangle_options_set_entry(). That explains your warning, the return value of gimp_rectangle_options_get_entry() is NULL because it was never set.
What's the state of this patch? Karine, are you still working on this?
None of the patches attached here apply any more. Removed serialization from some properties and cloaing as FIXED: 2006-11-05 Michael Natterer <mitch@gimp.org> * app/tools/gimprectangleoptions.c (gimp_rectangle_options_iface_base_init): remove GIMP_CONFIG_PARAM_SERIALIZE from the x0, y0, width, height, center-x and center-y properties. Fixes bug #346683.