GNOME Bugzilla – Bug 479999
Crop tool not updated when switching between portrait and landscape
Last modified: 2008-10-30 19:59:12 UTC
Using crop tool, making a rectangular selection; change between portrait and landscape, it's necessary to "redraw" another rectangle for changes to make effect. This is quite bizare because rectangle size writen in the tool properties is updated (100x200 -----[portrait / landscape]---> 200x100) and at that moment; size information is in contradiction with visible rectangle. Thanks Bruno Duyé
Thank you for taking the time to make a bug report. This behavior is intentional though (at least that's how I interpret the specification). The Selection and Crop tool specification [1] states "Only while rubber-banding (modifying) it shall be possible to enforce fixed ratio/widht/height/size. Right at the moment when both rubber-banding state and fixed mode become true, the fixed constraint shall be applied, [...]" This behavior makes sense for the Rectangle Select Tool (which shares code with the Crop tool), where you could want to make a 16:9 selection rectangle, and then add a 9:16 rectangle to the already existing rectangle selection. In this case you would not like the changing of landscape -> portrait to change the pending rectangle you have, since the new aspect is intended for the next rectangle you are going to create. You are welcome to discuss this on the gimp-developer mailing list, but changing this behavior is not likely to happen for GIMP 2.4, but it's not completely ruled out. [1] http://gui.gimp.org/index.php/Selection_%2B_crop_tool_specification
We should definitely discuss this with Peter. It seems to me that at least in the Crop tool it should be possible to quickly change aspect using the buttons. That's what they were added for in the first place.
Raising priority. We should get the interaction sorted out for 2.4.
first, I really would like to know a legitimate use case here for what is in effect experimenting with: "shall I cut it portrait or landscape?" I do not see it right now. What I see is an image, where the image content combined with users' goals makes it obvious if a portrait or landscape crop is needed. So tell me where this 'changing the orientation while the crop is pending" is useful, and I can make the decision what is the best interaction.
looking at 2.4rc3, I can see the cause of the bug report: we have gotten too clever about setting the default values of the fixed text field. crop tool + fixed ratio, I had specified that the default ratio gets set to the pending rectangle size. somebody thought that was a cool idea and I did not see at the time the usability bugs we get now. somebody programmed, contra to what is specced, that the spec that for crop/select tool + fixed size, the default size gets set to the pending rectangle size. these two things above cause the following usability bug: users cannot see clearly anymore that the number(s) in the fixed textfield are only a constraint that can be applied while adjusting the current rectangle. What happens is that _it_seems_ that the fixed textfield contains the current state of the current rectangle, which is not true. For that we have the X, Y, Width, Height fields. So this whole bug entry is caused by misleading users in the first place. I have corrected the spec. Please correct the code by setting the defaults as specced.
In reply to comment #4. We have had people on the list who said that the way they work with photographs is that they go through their pictures and use the Crop tool to select a more interesting region. They keep the aspect ratio constant for this, but sometimes they crop a portrait image out of a landscape one and vice versa. They would like to be able to try different sizes and also switch the orientation quickly. I am not sure if this is really an important usage scenario or not.
The use case is obviously the same as taking pictures with a camera, you sometimes can't immediately decide whether the scene should be taken portrait or landsape.
I can see the point of the last two posts (6+7). But I also expect that when you switch between portrait and landscape that certainly the position, if not also the size of the rect needs to be adjusted, to get a good result in an artistic way. the crop tool as specified simply works for this kind of workflow. we just need to clear up the confusion that is the root of this bug report. I posted in #5 how to do this.
That the default Fixed: Aspect becomes pending_rectangle_width:pending_rectangle_height is so that fixing the aspect for the crop tool should be possible (bug #355545). That the default Fixed: Size becomes pending_rectangle_width:pending_rectangle_height is because it to make it possible to fix the current size, and this change was made by me some time ago after a way of fixing the size had been requested. In the light of the insights provided in this bug, I now see that as a harmful hack. I agree on Peter's analysis that the reason this bug report exists in the first place is because we fool the users into believing that the default values in the Fixed: Aspect/Size entries denotes the current Aspect/Size of the rectangle, and I also agree that we should stop doing that. Changing this is dead-easy. It does however create two problems: 1. It is not possible to fix the aspect of a pending rectangle when resizing it (bug #355545) (or is there some way of achieving this that I look past?) 2. There is no way of conveniently fixing the current size (to prevent accidental resizing). IMO we should look into these problems before fixing this bug.
OK, I spend all day thinking about this bug and the solution is a bit more subtler than I wrote last night, so here we go: Yes, I think it is good to support keeping the current ratio of a pending rect while rubber-banding, for both crop and select tools (like the rest of the world does). But instead of showing a numerical values that confuse users, we show a word: "current", but only while there is a pending rectangle. The defaults for rectangle creation stay as-is. that keeps bug #355545 solved. I have documented this in the section: <a href="http://gui.gimp.org/index.php/Selection_%2B_crop_tool_specification#default_values">default values</a> Next, no tricks to prevent accidental resizing. If the current size is perfect, then commit the rect or click in the move handle that is 8 times bigger in size than the resize handles. working with the rects should be fast and flowing, not belts and braces. Last, to address the #2 comment: yes, there is a magic moment where it fully makes sense for both the crop and select tools to 'quickly change aspect using the buttons.' This moment is when the pending rect on the canvas fully reflects the fixed ratio/size settings in the tool options (not the other way around). I have documented this in the section: <a href="http://gui.gimp.org/index.php/Selection_%2B_crop_tool_specification#checkbox_and_the_shift_key">checkbox and the shift key</a>
Making good progress, hopefully I will be able to commit a fix for this today or tomorrow evening. Peter, two questions though: 1. Is the lowercase string "current" preferable over the capitalized form "Current"? Asking only to be sure; we don't want to change a string after it has been commited. 2. Now that a string is shown instead of numbers I don't see it as harmful to provide a way of fixing the current size, i.e. by having the default values in Fixed: Size be that of the pending rectangle when there is one. Before the planned change it was evil since it fooled users into believing the values therein represented the size of the current rectangle, but if we don't show numbers but instead the string "current" when the default size is used, why is it evil now? I'm pretty sure we will have complaints about there being no way to lock the current size if I remove this functionality.
re #11: 1) I have nothing against capitalisation here. Probably makes more people happy. Changed in the spec. 2) What problem are we trying to solve here? What else can one do with a perfectly sized rectangle then to either move it to the perfect position or to commit it, both of which are primary actions in the UI? Maybe I am unaware of some important workflow? For fixing the current ratio using the Fixed ratio constraint is the only way. Keeping the current size fixed normally does not involve the Fixed size constraint, nor the shift key.
I have commited the first part of the solution to this bug; making the string Current appear when the aspect of a current pending crop/selection rectangle is used for Fixed: Aspect ratio. I have removed the hack to enable fixing the size of rectangles. We'll see how this folds out. As it is now, the string Current is not automatically selected when giving the entry focus, for now we rely on the default behavior of the entry. I've tested the commited changes and it apears to work fine, so I will now look closer at the solution of the actual bug (making it possible to switch aspect of the pending rectangle) in the situation speced by Peter. Commited improvements to trunk, revision 23788. 2007-10-09 Martin Nordholts <martinn@svn.gnome.org> * app/tools/gimprectangleoptions.[ch]: Connect a new function gimp_rectangle_options_string_current_updates() that updates the Fixed: Aspect entry with a 'Current' string when aspect of the current pending rectangle is used, and sets sensitivity FALSE on aspect ratio changing buttons when that string is shown. Prevents the confusion mentioned in bug #479999. A new Rectangle Options property 'use-string-current' has been added that should be refactored away from the options object along with references to option widgets. * app/tools/gimprectangleselecttool.c (gimp_rect_select_tool_update_option_defaults): Set default Fixed: Aspect ratio to that of the pending rectangle, and always have default Fixed: Size as 100x100. * app/tools/gimpcroptool.c (gimp_crop_tool_update_option_defaults): Always have default Fixed: Size 100x100. * libgimpwidgets/gimpnumberpairentry.[ch] (gimp_number_pair_entry_class_init): Add a new property 'default-text' that contains text to be shown instead of numbers when default numbers are to be shown. (gimp_number_pair_entry_get_default_text) (gimp_number_pair_entry_set_default_text): Getter and setter for it. * libgimpwidgets/gimpwidgets.def: Updated.
That should do it. Fix commited to trunk, revision 23818. 2007-10-14 Martin Nordholts <martinn@svn.gnome.org> * app/tools/gimprectangletool.c (gimp_rectangle_tool_options_notify): When Fixed: Size/Aspect ratio numbers are swapped and the Fixed:-rule is active, swap width and height on any pending rectangle. Fixed bug #479999.
Thanks to all. I'll check svn to see the result. Bruno Duyé