Commit aec48714 authored by Jim Nelson's avatar Jim Nelson

#65/#1001: Undo/redo implemented for all appropriate commands. When undo/redo...

#65/#1001: Undo/redo implemented for all appropriate commands.  When undo/redo selected, switches back to page 
the operation occurred on.  Also detected during this patch that Pages were not always being freed when removed; 
this fixes that as well.
parent 3628f9d3
......@@ -308,6 +308,8 @@ public class CheckerboardLayout : Gtk.DrawingArea {
// subscribe to the new collection
view.contents_altered += on_contents_altered;
view.item_altered += on_item_altered;
view.item_metadata_altered += on_item_metadata_altered;
view.items_state_changed += on_items_state_changed;
view.items_visibility_changed += on_items_visibility_changed;
view.ordering_changed += on_ordering_changed;
......@@ -321,6 +323,8 @@ public class CheckerboardLayout : Gtk.DrawingArea {
~CheckerboardLayout() {
view.contents_altered -= on_contents_altered;
view.item_altered -= on_item_altered;
view.item_metadata_altered -= on_item_metadata_altered;
view.items_state_changed -= on_items_state_changed;
view.items_visibility_changed -= on_items_visibility_changed;
view.ordering_changed -= on_ordering_changed;
......@@ -329,11 +333,14 @@ public class CheckerboardLayout : Gtk.DrawingArea {
view.views_altered -= on_views_altered;
view.geometries_altered -= on_geometries_altered;
if (hadjustment != null && vadjustment != null) {
if (hadjustment != null)
hadjustment.value_changed -= on_viewport_shifted;
if (vadjustment != null)
vadjustment.value_changed -= on_viewport_shifted;
if (parent != null)
parent.size_allocate -= on_viewport_resized;
}
}
public void set_adjustments(Gtk.Adjustment hadjustment, Gtk.Adjustment vadjustment) {
......@@ -413,6 +420,14 @@ public class CheckerboardLayout : Gtk.DrawingArea {
schedule_background_reflow("on_contents_altered");
}
private void on_item_altered() {
schedule_background_reflow("on_item_altered");
}
private void on_item_metadata_altered() {
schedule_background_reflow("on_item_metadata_altered");
}
private void on_items_state_changed(Gee.Iterable<DataView> changed) {
if (!in_view)
return;
......
......@@ -12,22 +12,37 @@ public interface CommandDescription : Object {
// Command's overrideable action calls are guaranteed to be called in this order:
//
// * prepare()
// * execute() (once and only once)
// * prepare()
// * undo()
// * prepare()
// * redo()
// * prepare()
// * undo()
// * prepare()
// * redo() ...
//
// redo()'s default implementation is to call execute, which in many cases is appropriate.
public abstract class Command : Object, CommandDescription {
private string name;
private string explanation;
private weak CommandManager manager = null;
public Command(string name, string explanation) {
this.name = name;
this.explanation = explanation;
}
~Command() {
#if TRACE_DTORS
debug("Command dtor: %s/%s", name, explanation);
#endif
}
public virtual void prepare() {
}
public abstract void execute();
public abstract void undo();
......@@ -49,6 +64,17 @@ public abstract class Command : Object, CommandDescription {
public virtual string get_explanation() {
return explanation;
}
public weak CommandManager? get_command_manager() {
return manager;
}
// This should only be called by CommandManager.
public void internal_set_command_manager(CommandManager manager) {
assert(this.manager == null);
this.manager = manager;
}
}
public class CommandManager {
......@@ -74,6 +100,9 @@ public class CommandManager {
}
public void execute(Command command) {
// assign command to this manager
command.internal_set_command_manager(this);
// clear redo stack; executing a command implies not going to undo an undo
redo_stack.clear();
......@@ -87,6 +116,7 @@ public class CommandManager {
// update state before executing command
push(undo_stack, command);
command.prepare();
command.execute();
// notify after execution
......@@ -110,6 +140,7 @@ public class CommandManager {
push(redo_stack, command);
// undo command with state ready
command.prepare();
command.undo();
// report state changed after command has executed
......@@ -135,6 +166,7 @@ public class CommandManager {
push(undo_stack, command);
// redo command with state ready
command.prepare();
command.redo();
// report state changed after command has executed
......
......@@ -4,7 +4,47 @@
* See the COPYING file in this distribution.
*/
public abstract class SingleDataSourceCommand : Command {
// PageCommand stores the current page when a Command is created. Subclasses can call return_to_page()
// if it's appropriate to return to that page when executing an undo() or redo().
public abstract class PageCommand : Command {
private Page? page;
private bool auto_return = true;
public PageCommand(string name, string explanation) {
base (name, explanation);
page = AppWindow.get_instance().get_current_page();
if (page != null)
page.removed += on_page_removed;
}
~PageCommand() {
if (page != null)
page.removed -= on_page_removed;
}
public void set_auto_return_to_page(bool auto_return) {
this.auto_return = auto_return;
}
public override void prepare() {
if (auto_return)
return_to_page();
base.prepare();
}
public void return_to_page() {
if (page != null)
AppWindow.get_instance().set_current_page(page);
}
private void on_page_removed() {
get_command_manager().reset();
}
}
public abstract class SingleDataSourceCommand : PageCommand {
protected DataSource source;
public SingleDataSourceCommand(DataSource source, string name, string explanation) {
......@@ -26,7 +66,7 @@ public abstract class SingleDataSourceCommand : Command {
private void on_source_destroyed() {
// too much risk in simply removing this from the CommandManager; if this is considered too
// broad a brushstroke, can return to this later
AppWindow.get_command_manager().reset();
get_command_manager().reset();
}
}
......@@ -99,7 +139,7 @@ public abstract class GenericPhotoTransformationCommand : SingleDataSourceComman
}
}
public abstract class MultipleDataSourceCommand : Command {
public abstract class MultipleDataSourceCommand : PageCommand {
protected const int MIN_OPS_FOR_PROGRESS_WINDOW = 5;
protected Gee.ArrayList<DataSource> source_list = new Gee.ArrayList<DataSource>();
......@@ -150,7 +190,7 @@ public abstract class MultipleDataSourceCommand : Command {
// as with SingleDataSourceCommand, too risky to selectively remove commands from the stack,
// although this could be reconsidered in the future
if (source_list.contains(source))
AppWindow.get_command_manager().reset();
get_command_manager().reset();
}
public override void execute() {
......@@ -447,7 +487,7 @@ public class NewEventCommand : MultipleDataSourceCommand {
LibraryPhoto, SourceProxy?>();
public NewEventCommand(Gee.Iterable<DataView> iter) {
base(iter, _("Creating New Event..."), _("Removing Event..."), Resources.NEW_EVENT_LABEL,
base (iter, _("Creating New Event..."), _("Removing Event..."), Resources.NEW_EVENT_LABEL,
Resources.NEW_EVENT_TOOLTIP);
// get proxies for the photos' events as well as the key photo for the new event (which is
......@@ -510,7 +550,84 @@ public class NewEventCommand : MultipleDataSourceCommand {
}
private void on_proxy_broken() {
AppWindow.get_command_manager().reset();
get_command_manager().reset();
}
}
public class MergeEventsCommand : Command {
// Piggyback on a private command that deals with the lump group of all photos in all events
// (which needs to be processed before creating the command)
private class RealMergeEventsCommand : MultipleDataSourceCommand {
private SourceProxy master_event_proxy;
private Gee.HashMap<LibraryPhoto, SourceProxy?> old_photo_events =
new Gee.HashMap<LibraryPhoto, SourceProxy?>();
public RealMergeEventsCommand(Event master_event, Gee.Iterable<DataView> photos) {
base (photos, _("Merging..."), _("Unmerging..."), Resources.MERGE_LABEL,
Resources.MERGE_TOOLTIP);
master_event_proxy = master_event.get_proxy();
foreach (DataSource source in source_list) {
LibraryPhoto photo = (LibraryPhoto) source;
Event? event = photo.get_event();
SourceProxy? event_proxy = (event != null) ? event.get_proxy() : null;
old_photo_events.set(photo, event_proxy);
}
}
public override void execute_on_source(DataSource source) {
((LibraryPhoto) source).set_event((Event?) master_event_proxy.get_source());
}
public override void undo_on_source(DataSource source) {
LibraryPhoto photo = (LibraryPhoto) source;
SourceProxy? event_proxy = old_photo_events.get(photo);
photo.set_event(event_proxy != null ? (Event?) event_proxy.get_source() : null);
}
}
private RealMergeEventsCommand real_command;
public MergeEventsCommand(Gee.Iterable<DataView> iter) {
base (Resources.MERGE_LABEL, Resources.MERGE_TOOLTIP);
// the master event is the first one found with a name, otherwise the first one in the lot
Event master_event = null;
Gee.ArrayList<PhotoView> photos = new Gee.ArrayList<PhotoView>();
foreach (DataView view in iter) {
Event event = (Event) view.get_source();
if (master_event == null)
master_event = event;
else if (!master_event.has_name() && event.has_name())
master_event = event;
// store all photos in this operation; they will be moved to the master event
// (keep proxies of their original event for undo)
foreach (PhotoSource photo_source in event.get_photos())
photos.add(new PhotoView(photo_source));
}
assert(master_event != null);
assert(photos.size > 0);
real_command = new RealMergeEventsCommand(master_event, photos);
}
public override void prepare() {
real_command.prepare();
}
public override void execute() {
real_command.execute();
}
public override void undo() {
real_command.undo();
}
}
......@@ -161,6 +161,12 @@ public class DataCollection {
list.resort(get_order_added_comparator());
}
~DataCollection() {
#if TRACE_DTORS
debug("DataCollection dtor: %s", name);
#endif
}
public virtual string to_string() {
return "%s (%d)".printf(name, get_count());
}
......
......@@ -151,6 +151,7 @@ public abstract class DataSource : DataObject {
private Gee.ArrayList<DataView> subscribers = new Gee.ArrayList<DataView>();
private bool in_contact = false;
private bool marked_for_destroy = false;
private bool is_destroyed = false;
// This signal is fired at the end of the destroy() chain. The object's state is either fragile
// or unusable. It is up to all observers to drop their references to the DataObject.
......@@ -161,6 +162,12 @@ public abstract class DataSource : DataObject {
base (object_id);
}
~DataSource() {
#if TRACE_DTORS
debug("DataSource destroyed");
#endif
}
public override void notify_altered() {
// signal reflection
contact_subscribers(subscriber_altered);
......@@ -183,6 +190,18 @@ public abstract class DataSource : DataObject {
view.notify_metadata_altered();
}
public override void notify_membership_changed(DataCollection? collection) {
// DataSources can only be removed once they've been destroyed, and may not be re-added
// likewise
if (collection == null) {
assert(is_destroyed);
} else {
assert(!is_destroyed);
}
base.notify_membership_changed(collection);
}
// If a DataSource cannot produce snapshots, return null.
public virtual SourceSnapshot? save_snapshot() {
return null;
......@@ -205,6 +224,9 @@ public abstract class DataSource : DataObject {
// clear the subscriber list
subscribers.clear();
// mark as destroyed
is_destroyed = true;
// propagate the signal
destroyed();
}
......@@ -475,6 +497,12 @@ public class DataView : DataObject {
// first notification of destruction.
source.internal_subscribe(this);
}
~DataView() {
#if TRACE_DTORS
debug("DataView destroyed");
#endif
}
public override string get_name() {
return "View of %s".printf(source.get_name());
......
......@@ -320,6 +320,12 @@ public class Event : EventSource, Proxyable {
return "Event [%lld/%lld] %s".printf(event_id.id, get_object_id(), get_name());
}
public bool has_name() {
string raw_name = get_raw_name();
return raw_name != null && raw_name.length > 0;
}
public override string get_name() {
string event_name = event_table.get_name(event_id);
......
......@@ -123,8 +123,8 @@ public class EventsDirectoryPage : CheckerboardPage {
//
// merge tool
merge_button = new Gtk.ToolButton.from_stock(Gtk.STOCK_ADD);
merge_button.set_label(_("Merge"));
merge_button.set_tooltip_text(_("Merge into a single event"));
merge_button.set_label(Resources.MERGE_LABEL);
merge_button.set_tooltip_text(Resources.MERGE_TOOLTIP);
merge_button.clicked += on_merge;
merge_button.sensitive = (get_view().get_selected_count() > 1);
merge_button.is_important = true;
......@@ -170,8 +170,8 @@ public class EventsDirectoryPage : CheckerboardPage {
actions += rename;
Gtk.ActionEntry merge = { "Merge", Gtk.STOCK_ADD, TRANSLATABLE, "<Ctrl>M", TRANSLATABLE, on_merge };
merge.label = _("Merge Events");
merge.tooltip = _("Merge into a single event");
merge.label = Resources.MERGE_MENU;
merge.tooltip = Resources.MERGE_TOOLTIP;
actions += merge;
return actions;
......@@ -257,57 +257,9 @@ public class EventsDirectoryPage : CheckerboardPage {
private void on_merge() {
if (get_view().get_selected_count() <= 1)
return;
LibraryWindow.get_app().set_busy_cursor();
Event master_event = ((EventDirectoryItem) get_view().get_selected_at(0)).event;
bool has_name = (master_event.get_raw_name() != null && master_event.get_raw_name() != "");
// list of photos to merge
Gee.ArrayList<LibraryPhoto> photos = new Gee.ArrayList<LibraryPhoto>();
// must iterate through all events before touching photos (which can delete events)
foreach (DataView view in get_view().get_selected()) {
Event event = ((EventDirectoryItem) view).event;
if (!has_name && event.get_raw_name() != null && event.get_raw_name() != "") {
master_event.rename(event.get_raw_name());
has_name = true;
}
foreach (PhotoSource photo in event.get_photos()) {
if (photo is LibraryPhoto)
photos.add((LibraryPhoto) photo);
}
}
int count = 0;
int total = photos.size;
Cancellable cancellable = null;
ProgressDialog progress = null;
if (total >= MIN_PHOTOS_FOR_PROGRESS_WINDOW) {
cancellable = new Cancellable();
progress = new ProgressDialog(AppWindow.get_instance(), _("Merging..."), cancellable);
}
// add each photo to master event
foreach (LibraryPhoto photo in photos) {
photo.set_event(master_event);
if (progress != null) {
progress.set_fraction(++count, total);
spin_event_loop();
if (cancellable.is_cancelled())
break;
}
}
if (progress != null)
progress.close();
LibraryWindow.get_app().set_normal_cursor();
MergeEventsCommand command = new MergeEventsCommand(get_view().get_selected());
get_command_manager().execute(command);
}
}
......
......@@ -578,7 +578,7 @@ public class LibraryWindow : AppWindow {
private void import_queue_batch_finished() {
if (displaying_import_queue_page && import_queue_page.get_batch_count() == 0) {
remove_page(import_queue_page);
remove_page(import_queue_page, library_page);
displaying_import_queue_page = false;
}
}
......@@ -730,7 +730,7 @@ public class LibraryWindow : AppWindow {
!(old_parent.get_month() == Time.local(event.get_start_time()).month &&
old_parent.get_year() == Time.local(event.get_start_time()).year)) {
// remove from sidebar
remove_event_tree(stub, false);
sidebar.remove_page(stub);
// add to sidebar again
sidebar.insert_child_sorted(get_parent_page(stub.event).get_marker(), stub,
......@@ -819,50 +819,46 @@ public class LibraryWindow : AppWindow {
foreach (EventPageStub stub in event_list) {
if (stub.event.equals(event)) {
event_stub = stub;
break;
}
}
if (event_stub == null)
return;
// remove from sidebar
remove_event_tree(event_stub, true);
// jump to the Photos page if current page is being deleted
if (get_current_page() is EventPage)
switch_to_library_page();
else
sidebar.place_cursor(get_current_page());
remove_event_tree(event_stub);
// jump to the Events page
if (event_stub.has_page() && event_stub.get_page() == get_current_page())
switch_to_events_directory_page();
}
private void remove_event_tree(SidebarPage page, bool permanent_remove) {
// remove from notebook
if (page is PageStub && ((PageStub) page).has_page() && permanent_remove)
remove_from_notebook(((PageStub) page).get_page());
private void remove_event_tree(PageStub stub) {
// grab parent page
SidebarPage parent = sidebar.get_parent_page(page);
// remove from sidebar
sidebar.remove_page(page);
SidebarPage parent = sidebar.get_parent_page(stub);
// remove from notebook and sidebar
remove_stub(stub);
// remove parent if empty
if (parent != null && !(parent is MasterEventsDirectoryPage)) {
assert(parent is PageStub);
if (!sidebar.has_children(parent.get_marker()))
remove_event_tree(parent, true);
remove_event_tree((PageStub) parent);
}
if (page is SubEventsDirectoryPageStub) {
// remove from appropriate list
if (stub is SubEventsDirectoryPageStub) {
// remove from events directory list
bool removed = events_dir_list.remove((SubEventsDirectoryPageStub) page);
bool removed = events_dir_list.remove((SubEventsDirectoryPageStub) stub);
assert(removed);
debug("Removed page %s", page.get_page_name());
} else if (page is EventPageStub && permanent_remove) {
} else if (stub is EventPageStub) {
// remove from the events list
bool removed = event_list.remove((EventPageStub) page);
bool removed = event_list.remove((EventPageStub) stub);
assert(removed);
debug("Removed page %s", page.get_page_name());
}
}
......@@ -886,7 +882,7 @@ public class LibraryWindow : AppWindow {
// remove from page table and then from the notebook
ImportPage page = camera_pages.get(camera.uri);
camera_pages.unset(camera.uri);
remove_page(page);
remove_page(page, library_page);
// if no cameras present, remove row
if (CameraTable.get_instance().get_count() == 0 && cameras_marker != null) {
......@@ -945,7 +941,7 @@ public class LibraryWindow : AppWindow {
add_to_notebook(orphan);
}
private void remove_page(Page page) {
private void remove_page(Page page, Page fallback_page) {
// a handful of pages just don't go away
assert(page != library_page);
assert(page != events_directory_page);
......@@ -961,7 +957,19 @@ public class LibraryWindow : AppWindow {
// switch away if necessary to collection page (which is always present)
if (get_current_page() == page)
switch_to_library_page();
switch_to_page(fallback_page);
}
private void remove_stub(PageStub stub) {
// see note in remove_page() for why this is done
if (stub.has_page()) {
pages_to_be_removed.add(stub.get_page());
Idle.add(remove_page_internal);
}
// remove stub (which holds a marker) from the sidebar
sidebar.remove_page(stub);
debug("Removed stub %s", stub.get_name());
}
private bool remove_page_internal() {
......@@ -976,6 +984,8 @@ public class LibraryWindow : AppWindow {
debug("Removed page %s", page.get_page_name());
page.notify_removed();
pages_to_be_removed.remove_at(0);
}
......@@ -1050,6 +1060,11 @@ public class LibraryWindow : AppWindow {
switch_to_page(start_page);
}
public override void set_current_page(Page page) {
// switch_to_page() will call base.set_current_page(), maintain the semantics of this call
switch_to_page(page);
}
public void switch_to_page(Page page) {
if (page == get_current_page())
return;
......@@ -1109,7 +1124,7 @@ public class LibraryWindow : AppWindow {
// do this prior to changing selection, as the change will fire a cursor-changed event,
// which will then call this function again
set_current_page(page);
base.set_current_page(page);
Idle.add_full(Priority.HIGH, place_sidebar_cursor);
......
......@@ -42,7 +42,7 @@ public abstract class Page : Gtk.ScrolledWindow, SidebarPage {
private ViewCollection view = null;
private Gtk.Window container = null;
private PageLayout layout = null;
private Gtk.MenuBar menu_bar = null;
private string menubar_path = null;
private SidebarMarker marker = null;
private Gdk.Rectangle last_position = Gdk.Rectangle();
private Gtk.Widget event_source = null;
......@@ -53,6 +53,9 @@ public abstract class Page : Gtk.ScrolledWindow, SidebarPage {
private bool report_resize_finished = false;
private Gdk.Point last_down = Gdk.Point();
public virtual signal void removed() {
}
public Page(string page_name) {
this.page_name = page_name;
this.view = new ViewCollection("ViewCollection for Page %s".printf(page_name));
......@@ -63,7 +66,24 @@ public abstract class Page : Gtk.ScrolledWindow, SidebarPage {
}
~Page() {
#if TRACE_DTORS
debug("Page destroyed: %s", page_name);
#endif
}
// This is called by the page controller when it has removed this page ... pages should override
// this (or the signal) to clean up
public virtual void notify_removed() {
// untie signals
detach_event_source();
view.halt_monitoring();
// remove refs to external objects which may be pointing to the Page
layout = null;
clear_marker();
clear_container();
removed();
}
public string get_page_name() {
......@@ -92,7 +112,6 @@ public abstract class Page : Gtk.ScrolledWindow, SidebarPage {
}
public virtual void set_container(Gtk.Window container) {
// this should only be called once
assert(this.container == null);
this.container = container;
......@@ -164,7 +183,9 @@ public abstract class Page : Gtk.ScrolledWindow, SidebarPage {
}
public virtual Gtk.MenuBar get_menubar() {
return menu_bar;
assert(menubar_path != null);
return (Gtk.MenuBar) ui.get_widget(menubar_path);
}
public abstract Gtk.Toolbar get_toolbar();
......@@ -247,15 +268,14 @@ public abstract class Page : Gtk.ScrolledWindow, SidebarPage {
}
protected void init_ui_bind(string? menubar_path) {
this.menubar_path = menubar_path;
ui.insert_action_group(action_group, 0);
common_action_group = new Gtk.ActionGroup("CommonActionGroup");
AppWindow.get_instance().add_common_actions(common_action_group);
ui.insert_action_group(common_action_group, 0);
if (menubar_path != null)
menu_bar = (Gtk.MenuBar) ui.get_widget(menubar_path);
ui.ensure_update();
}
......
......@@ -112,6 +112,10 @@ along with Shotwell; if not, write to the Free Software Foundation, Inc.,
public const string NEW_EVENT_MENU = _("_New Event");
public const string NEW_EVENT_LABEL = _("New Event");
public const string NEW_EVENT_TOOLTIP = _("Create new event from the selected photos");
public const string MERGE_MENU = _("_Merge Events");
public const string MERGE_LABEL = _("Merge");
public const string MERGE_TOOLTIP = _("Merge into a single event");
private Gtk.IconFactory factory = null;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment