GNOME Bugzilla – Bug 761587
Centralize action toggle logic
Last modified: 2016-02-29 19:50:05 UTC
The toggle logic for the actions is spread out across photo-application.c and photo-selection-toolbar.c files. By centralizing them at one place (photos-application) it'll be easier to keep them consistent and correct. Right now, e.g, some actions can be applied through accelerators before the item is fully loaded, leading to bad things.
Created attachment 320624 [details] [review] Move print toggle logic from selection-toolbar to photos-application.
Created attachment 320625 [details] [review] Move delete toggle logic to photos-application.
Created attachment 320626 [details] [review] - Move open toggle logic to photos-application. - Add accelerators for open.
Created attachment 320627 [details] [review] Move properties toggle logic to photos-application.
Review of attachment 320624 [details] [review]: This is fantastic. Thanks, Rafael. I pushed it after making the following adjustment. ::: src/photos-application.c @@ +344,3 @@ && !photos_base_item_is_collection (item))); g_simple_action_set_enabled (priv->save_action, enable); + g_simple_action_set_enabled (priv->print_action, enable); Would be slightly nice to keep them alphabetical.
Created attachment 320641 [details] [review] application, selection-toolbar: Use the GAction for printing
Review of attachment 320625 [details] [review]: Looks quite except a few smaller details. By the way, it is better to have the high-level / visible change in the commit message. eg., in this case, the visible change is that we are disabling the delete action in the preview during loading and if the item can't be deleted. ::: src/photos-application.c @@ +301,3 @@ PhotosWindowMode mode; gboolean enable; + gboolean in_selection_mode; Nitpick: selection_mode for consistency. Consistency is good because it helps when grepping. @@ +357,3 @@ + + selection = photos_selection_controller_get_selection (priv->sel_cntrlr); + enable = ((load_state == PHOTOS_LOAD_STATE_FINISHED && mode == PHOTOS_WINDOW_MODE_PREVIEW) When we are in PREVIEW, we should also check if the item can be deleted using photos_base_item_can_trash. eg., remote items can't be deleted. Otherwise, if you delete and wait for the timer to expire, it will crash. @@ +362,3 @@ + { + const gchar *urn = l->data; + item = PHOTOS_BASE_ITEM (photos_base_manager_get_object_by_id (priv->state->item_mngr, urn)); It will be better to use a different local variable instead of 'item'. 'item' has the selection_or_active_item, and if we add another block of conditions that needs it, then it won't work because we are overwriting it here.
Created attachment 320645 [details] [review] Disable deleting from the preview while loading and if unsupported Made the above adjustments.
Review of attachment 320626 [details] [review]: ::: src/photos-application.c @@ +1362,3 @@ const gchar *search_accels[2] = {"<Primary>f", NULL}; const gchar *select_all_accels[2] = {"<Primary>a", NULL}; + const gchar *open_current_accels[2] = {"<Primary>o", NULL}; While I like the idea of having ctrl+o as a shortcut for opening in a different application, I would request you to not do this right now. The reason is that "opening in a different application" is a bit broken at this moment with respect to editing: Say, you have edited an item and you open it in a different application. You won't get the edited copy because the edit operations are kept in memory, and the original is never touched (because that is how non-destructive editing is supposed to be). Of course, you can get the edited version using export, but we don't show those in the app. Right now, export is a kludge to interop with nautilus and other apps because we are missing a "sharing framework". Therefore, for the immediate short-term, we might end up temporarily disabling the "open in another app" feature until we figure out a way to do a "rich export" from gnome-photos to gimp and darktable. That is why I want to avoid more user visible controls for this feature. In any case, when we add a new shortcut, it needs to be added to photos-help-overlay.ui too. ::: src/photos-selection-toolbar.c @@ +430,3 @@ + G_CALLBACK (photos_selection_toolbar_open), + self, + G_CONNECT_SWAPPED); Can't this be merged with photos_application_open_current? We could check which mode we are in and either use a loop or just the active item.
Review of attachment 320627 [details] [review]: Looks quite good, except one small thing: ::: src/photos-application.c @@ +868,3 @@ + + gtk_widget_destroy (GTK_WIDGET (dialog)); + if (photos_selection_controller_get_selection_mode (priv->sel_cntrlr)) No need to check this. We don't check it in other places, and it should be robust about these things.
Created attachment 320649 [details] [review] Move open toggle logic to photos-application New version with proposed fixes.
Created attachment 320650 [details] [review] Move properties toggle logic to photos-application New version with proposed fixes.
Created attachment 320672 [details] [review] Move open toggle logic to photos-application
Created attachment 320673 [details] [review] Move properties toggle logic to photos-application Forgot to rebase.
Created attachment 321436 [details] [review] Move open toggle logic to photos-application Rebased against master.
Created attachment 321437 [details] [review] Move properties toggle logic to photos-application rebased against master.
Review of attachment 321436 [details] [review]: Nice patch. Looks good apart from some minor things: ::: src/photos-application.c @@ +363,3 @@ + + if (photos_base_item_get_default_app_name (selected_item) != NULL) + show_open = TRUE; It would be slightly more readable to re-arrange the trash/delete logic so that we don't use enable inside the loop. @@ +810,3 @@ + + item = PHOTOS_BASE_ITEM (photos_base_manager_get_object_by_id (priv->state->item_mngr, urn)); + time = gtk_get_current_event_time (); Nitpick: We don't really need to call this in every iteration of the loop because there is only one GdkEvent corresponding to the mouse click or key press. (It used to be inside the loop in SelectionToolbar because the variable was not needed elsewhere.) @@ +816,3 @@ + else + { + item = photos_application_get_selection_or_active_item (self); Let's use photos_base_manager_get_active_object because it is more direct and we don't want to look at the selection at all. This could, in theory, hide a programming error where we still have a selection in the preview. :)
Created attachment 321501 [details] [review] application: Shuffle some code around
Created attachment 321502 [details] [review] application, selection-toolbar: Use the GAction for opening Pushed after making the above adjustments.
(In reply to Rafael Fonseca from comment #16) > Created attachment 321437 [details] [review] [review] > Move properties toggle logic to photos-application > > rebased against master. Any comments on this patch? It's the last one in the series after which we can close this bug.
Created attachment 322699 [details] [review] application, selection-toolbar: Use the GAction for properties Rebased on top of master and pushed.
Thanks for all the patches, Rafael!