GNOME Bugzilla – Bug 373060
allow to easily switch to last used tool
Last modified: 2018-05-24 12:03:13 UTC
When using two tools and swapping between them often, instead of having to press the other tool's key, there should be a "go to previous tool key" which swaps back to the last-used tool. Other information: Corel apps have this feature, where it is loved! Photoshop doesn't have it!
This sounds useful. tomr, can you tell us what keybinding other apps use for this? If the key is still available, we should probably use the same. Setting the gnome-love keyword as this is a relatively simple change and it would be a good introduction into the GIMP UI code.
Hi Sven, Apparently Corel Photo Paint uses spacebar for swapping to previous tool. Of course that is already in use ... so maybe ` (backquote)? It's easy to hit and I don't think it's in use in the default GIMP keybindings :)
Backquote is hard to hit and a dead key in many layouts.
What about backspace?
Backspace seems like it should delete/erase/clear something. How about shift-space, or \ or / ? I haven't used the default keys in years so I'm not sure what is available.
Shift-X might work - swapping fg/bg color is x and this is kind of similiar. Didn't we use to have that kind of tool swapping ages ago?
Created attachment 150422 [details] [review] Add swap tool function and actions. Keeps a reference to last used tool. <shift>X or tools-menu to try it out.
Created attachment 150424 [details] [review] git format-patch output
Created attachment 150425 [details] [review] git format-patch output, 2nd file.
Created attachment 150427 [details] [review] same patch again, squashed. Sorry for spamming.
Created attachment 150429 [details] [review] 0001-Bug-373060-allow-to-easily-switch-to-last-used-tool.patch This is a cleaned up version, rearranging code slightly, adding a help-id, cleaning up commit message. I only know of one thing that should be fixed: When the toolbox is the active window tool-swapping doesn't work. It would also be nice to get a test-case added for tool swapping to test-ui.c. But overall, a very well written patch.
Created attachment 150456 [details] [review] 0001-Bug-373060-allow-to-easily-switch-to-last-used-tool.patch gimp_context_swap_tools() now finds context where GIMP_CONTEXT_PROP_TOOL is defined, then swaps context->tool_info and context->last_tool_info. It corrects the wrong behavior when toolbox has the focus. I need advice : I did not define last_tool_info as a property with proper getter, setter and name, as I think it shouldn't be accessed directly. It is currently implemented as a kept reference on the previous active GimpToolInfo, is it a good design ? Further work : test-case
Digging deeper into the code, this feature appears to need more love than what I did, it's currently too short-sighted - not speaking about the GLib-blind code I produced. I think this feature should backup and restore a snapshot of the entire active GimpContext, not only the GimpTool. User Interaction : - Keyboard shortcut : <shift>X - On first use, the active context is backed up but remains active. - On next uses, the active context is backed up, and previously saved context is restored. This would allow a more complete user-defined tool swapping instead of just swapping different GimpTools back and forth. For now, please disregard this patch. Damien.
Created attachment 166256 [details] [review] Allow to backup and swap contexts When the user hits <Shift>X, the active context is backed up. When the user hits <Shift>X again, the active context is swapped with the backed up context. -Joshua
Review of attachment 166256 [details] [review]: Please adjust your patch to follow the GIMP coding style, in particular the naming convention for variables and where spaces should be used. I think we also need to discuss how to present this to the user. "Swap Contexts" is not the most intuitive menu item. It needs a tool-tip at least.
Also, keeping the swap context around in GimpContext is not the right approach IMHO. This should probably be a property and member of the Gimp object.
Created attachment 192570 [details] [review] patch I took the reviewed patch from Damien de Lemeny and adopted it to the current tree. Corrected some erroneous glib-handling and renamed the menu entry from "Swap Tools" to "Last Tool" which might be more comprehensible. Separated the reference-keeping into a distinct function-call. Added a test-case "swap-tools" in test-ui.c which should be a first approach.
Created attachment 254330 [details] [review] Bug 373060 - allow to easily switch to last used tool. Hey, I saw this patch (attachment 192570 [details] [review]) sadly sitting around, so I took on me to review it. I actually saw afterwards that there were 2 implementations here, and I only reviewed the one where only the GimpToolInfo is saved (because it was in the last comment by Alexander Hämmerle), not the whole context. But it still works great, and I guess this implementation makes sense, saving only a reference to the last tool_info. Why would we want to reference the whole context when a user only switched tool? Things like brush or dynamics still follows whatever "Shared Options" is ticked or not in the preferences. So it is good. If I missed something and we really should save the whole context, tell me and I'll review the previous version of the patch (but for my inner-laziness, I would prefer not :p). Anyway the attached patch fixes a bunch of dead spaces and some ref/unref bugs which were hanging around (actually sorry Alexander, but the fixes you made to the original patches about g_object_ref/unref were not right as far as I could make logics to this all. I realized after fixing that I basically reverted several of your changes to how it was on Damien's patch. Thanks anyway for the test case!). I also fixed and made robuster the test in test-ui.c to fit our current code, which is good because it made me discover this part of our codebase. Out of topic: while I was at it, I discovered that the tests "automatic_tab_style" and "restore_recently_closed_multi_column_dock" fail. Are these tests actually still used if nobody ever saw this until now?
Created attachment 254674 [details] [review] Bug 373060 - allow to easily switch to last used tool. Small update of my last patch.
Nice, and I'm all for it, but it already hurts that GimpContext contains the tool at all, can we please use the last tool out there and instead use some local hack elsewhere, e.g. in tool_manager.c or tools-commands.c?
...take the last tool out there...
Ok. I'll have a look. Damn my inner-laziness is sad. :p
Hey Mitch, I've had a look. From what you said above, so don't we want to have the tool associated with a context? I feel they are pretty related. But I may miss some concept there, so I don't know. To be honest, on a user perspective, tools and context management are extremely messy in GIMP (even more when multi-devices come in the game). Aryeom and I are often lost on various tool-management issues, and we have to randomly grope to find solutions. And I had feedbacks from other users who were kind of lost too (and there are a bit of bug report about similar issues). That may be worth to review some of the paradigm about the relationships between context, tools, and devices, no? Better default, improved usability and improved capabilities could be worth it. :-) For a start, if really you believe tools should not be tied to the context, that's easy to change and to fully move the tool into the tool_manager (right now, the tool information is kind of duplicated in both class, which is not a nice design). Should I do it?
The tool being part of the context is just too useful :) Things can be a bit ugly and at the same time parctical, let's leave it as is. I'm juat talking about the last tool, there is no reason to pollute all the gazillions of contexts we keep around with that information.
Does this bug still need something done ? Could someone please mention what needs to be done ?
AbdealiJK > I need to edit my patch to account for Michael Natterer review. Unfortunately I had no time lately for this specific issue. I'll see when I can come back to it.
We should decide whether this is something we can and can do for 2.10, or move it to a future milestone.
I will try to look at my branch, when I can, which must still be lost somewhere in my locale repo.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gimp/issues/220.