Remember location of editing tool windows
Submitted by an unknown user
Link to original bug (#719198)
Description
---- Reported by shotwell-maint@gnome.bugs 2013-07-22 14:33:00 -0700 ----
Original Redmine bug id: 7264
Original URL: http://redmine.yorba.org/issues/7264
Searchable id: yorba-bug-7264
Original author: Wolfgang Steitz
Original description:
The adjust tool window is rather huge and blocks large parts of the photo, so the first thing I do is moving it to the side. When editing several photos, this gets quite annoying. I would really love if shotwell persists the location of the adjust tool. For the other tools this is not so much of a problem, since the tool windows are smaller.
The attached patch is a first shot at implementing this. The last location of the tool window is saved via gsettings, so the location is persisted even between shotwell sessions. If you are interested in this functionality, I would like to work on this and have some questions:
-
Should the location be persisted between sessions (gsettings) or rather just be saved for the current session?
-
The patch does not work in the photo viewer mode and in fullscreen mode. Would we use the same saved location there?
After implementing this with gsettings, I think just saving the location for one session might make things a bit easier.
---- Additional Comments From shotwell-maint@gnome.bugs 2013-08-27 12:26:00 -0700 ----
History
Comment 1
Updated by Wolfgang Steitz 4 months ago
- File remember_adjusttool_location.patch added
-
Assignee deleted (
<strike>
_Wolfgang Steitz_</strike>
)
Comment 2
Updated by Jim Nelson 4 months ago
- Subject changed from Remember location of adjust tool windo to Remember location of editing tool windows
I think the broader question is whether to remember the location of all editing windows: Crop, Straighten, Red-Eye, and Adjust. It seems to me that we should do this for all of them. Not that all of them should use the same remembered location, but that each is remembered.
In the past we've broken this up by first saving the location in memory (hence only for the current session) and then later using GSettings to persist them. I think that makes sense here too.
Comment 3
Updated by Wolfgang Steitz 4 months ago
- File 7264_v2.patch added
Attached an updated version of the patch that implements remembering the position of all editing windows. The last location of each tool is stored in a hashmap, so only for the current session.
I tried to get the class name of an editing tool (e.g. "AdjustTool") from the object, but that was not successful. So I added a name variable to the EditingTool class.
My next goal is to get this working in viewer-mode and fullscreen.
What do you think?
Comment 4
Updated by Jim Nelson 4 months ago
- Status changed from Open to Review
We'll take a look at it soon. If you want to continue working on the patch until then, please go ahead. I think having this work for both windowed and fullscreen mode would be great.
As far as class name, I would think .get_type().name()
would do the trick.
Comment 5
Updated by Jim Nelson 4 months ago
- Target version set to 0.15.0
Comment 6
Updated by Wolfgang Steitz 4 months ago
- File 7264_v3.patch added
.get_type() is what I tried, but that gives "The name get_type' does not exist in the context of
EditingTools.EditingTool?'", same for .get_class()...
The current iteration seems to work fine in windowed and fullscreen mode.
Comment 7
Updated by Jim Nelson 4 months ago
- File 7264-4.diff added
- Status changed from Review to Open
- Assignee set to Wolfgang Steitz
I think the get_type() / get_class() problem stems from EditingTool not inheriting from Object. Either way, your solution is fine, so let's stick with that. Some changes I made to your patch:
- The if/else if/else in PhotoPage.place_tool_window() wasn't following our guidelines, so I cleaned it up. I think the logic is still correct, it would be helpful if you could look over it.
- Because EditingTool.get_tool_window() can return null, need to check for that in deactivate_tool. However, by not checking for it your patch uncovered a couple of bugs in StraightenTool, which I've corrected. (Thanks!)
One issue remains: if the user moves the Shotwell window, the activated tool window positions remain fixed in their old locations, even if the user didn't move them. This doesn't feel right, especially on a multiple monitor setup (i.e. activate Adjust, close it, move Shotwell to other monitor, activate Adjust, and the adjust window is on the original monitor). My suggestion: if the main window is moved, clear the locations Map. Not perfect, but better than existing behavior.
I've attached my revised patch to this ticket. Please work from this patch going forward. Thanks!
Comment 8
Updated by Wolfgang Steitz 3 months ago
- File 7264_5.diff added
Moving and resizing really felt strange. As you suggested, I just reset the map in that case. And if the user has not changed the position of the editing window, I do not store the location at all.
Comment 9
Updated by Jim Nelson 3 months ago
- Status changed from Open to Review
Comment 10
Updated by Wolfgang Steitz 3 months ago
- Status changed from Review to Fixed
Applied in changeset 9d673539.
Comment 11
Updated by Jim Nelson 3 months ago
Thanks!
--- Bug imported by chaz@yorba.org 2013-11-25 22:00 UTC ---
This bug was previously known as bug 7264 at http://redmine.yorba.org/show_bug.cgi?id=7264 Imported an attachment (id=262745) Imported an attachment (id=262746) Imported an attachment (id=262747) Imported an attachment (id=262748) Imported an attachment (id=262749)
Unknown milestone "unknown in product shotwell. Setting to default milestone for this product, "---". Setting qa contact to the default for this product. This bug either had no qa contact or an invalid one.
Version: 0.15.0
Resolution: RESOLVED FIXED