GNOME Bugzilla – Bug 743096
Migrate from GtkAction to GAction
Last modified: 2015-02-07 09:32:08 UTC
Migrate from the deprecated GtkAction to GAction. AlmanahApplication currently use GAction for the App menu, but the actions from the toolbar in AlmanahMainWindow still use the old GtkAction API.
Created attachment 295892 [details] [review] App: Migrated GtkUIManager/GtkAction to GAction All the stuff related to GtkUIManager and GActions has been removed to support the modern GAction framework. The actions in data/almana.ui has been added as main window actions, with the "win." prefix, and keyboard accelerators has been moved to the application.
Review of attachment 295892 [details] [review]: Very brief review. ::: src/main-window.c @@ +676,3 @@ GtkTextTag *tag; gchar *tag_name; + gchar *action_name = NULL; I think this can be const, and the g_strdup()s omitted. @@ +968,3 @@ * their sensitivity set by the code. */ + if (priv->updating_formatting == TRUE) { + g_simple_action_set_state (action, parameter); Could you not move the g_simple_action_set_state() call to the top of the function and remove the duplicates from the code below? @@ +1188,3 @@ g_object_unref (entry); + /* TODO: Should be deactivated all the toolbar actions? */ TODO here. ::: src/widgets/calendar-button.c @@ +383,3 @@ + AlmanahCalendarButton *self = ALMANAH_CALENDAR_BUTTON (user_data); + + g_action_group_activate_action (G_ACTION_GROUP (self->priv->main_window), "select-date", NULL); This seems like a violation of modularity. The main window should connect to the calendar button, as it owns the button; not the other way round.
Thanks Philip for your review. (In reply to comment #2) > Review of attachment 295892 [details] [review]: > > Very brief review. > > ::: src/main-window.c > @@ +676,3 @@ > GtkTextTag *tag; > gchar *tag_name; > + gchar *action_name = NULL; > > I think this can be const, and the g_strdup()s omitted. Yes. > @@ +968,3 @@ > * their sensitivity set by the code. */ > + if (priv->updating_formatting == TRUE) { > + g_simple_action_set_state (action, parameter); > > Could you not move the g_simple_action_set_state() call to the top of the > function and remove the duplicates from the code below? I know that looks odd, but the action state must be changed just in three cases and not always: 1. when the text under the cursor changes (the piece of code that you point), 2. when doesn't meets 1, the action is toggled to true and the user accepts the dialog, and 3. when the action doesn't meets 1 and is toggle to false. But I think that it's possible to clean the code with a boolean, a tag and a goto call... I'll give it a try. > > @@ +1188,3 @@ > g_object_unref (entry); > > + /* TODO: Should be deactivated all the toolbar actions? */ > > TODO here. Good point. > > ::: src/widgets/calendar-button.c > @@ +383,3 @@ > + AlmanahCalendarButton *self = ALMANAH_CALENDAR_BUTTON (user_data); > + > + g_action_group_activate_action (G_ACTION_GROUP (self->priv->main_window), > "select-date", NULL); > > This seems like a violation of modularity. The main window should connect to > the calendar button, as it owns the button; not the other way round. Yes, you're right, I'll append two new actions to CalendarButton... (lazy mode off).
Created attachment 296049 [details] [review] App: Migrated GtkUIManager/GtkAction to GAction All the stuff related to GtkUIManager and GActions has been removed to support the modern GAction framework. The actions in data/almana.ui has been added as main window actions, with the "win." prefix, and keyboard accelerators has been moved to the application.
Review of attachment 296049 [details] [review]: ::: src/main-window.c @@ +713,3 @@ + if (tag_name) + g_free (tag_name); g_free(NULL) is safe, so there’s no need to add a check here. @@ +1197,3 @@ + NULL + }; + gint i = 0; Should be a guint. @@ +1220,3 @@ + future_entry = almanah_entry_get_editability (priv->current_entry) == ALMANAH_ENTRY_FUTURE; + gtk_text_view_set_editable (priv->entry_view, !future_entry); + while (affected_actions[i] != NULL) { This should be a for-loop: for (i = 0; affected_actions[i] != NULL; i++) ::: src/widgets/calendar-button.c @@ +103,3 @@ + "Main Window", "Almanah main window.", + GTK_TYPE_APPLICATION_WINDOW, + G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); This main-window property (and all associated code) could be removed now. @@ +131,3 @@ + NULL, NULL, + NULL, + G_TYPE_NONE, 0); Why does this need to be exposed? Can’t you handle this internally in the CalendarButton widget and update the widget to select today? At the moment, this signal is emitted, MainWindow handles it, and then just calls almanah_calendar_button_select_date().
(In reply to comment #5) > Review of attachment 296049 [details] [review]: > > ::: src/main-window.c > @@ +713,3 @@ > > + if (tag_name) > + g_free (tag_name); > > g_free(NULL) is safe, so there’s no need to add a check here. yes. > @@ +1197,3 @@ > + NULL > + }; > + gint i = 0; > > Should be a guint. yes. > @@ +1220,3 @@ > + future_entry = almanah_entry_get_editability (priv->current_entry) == > ALMANAH_ENTRY_FUTURE; > + gtk_text_view_set_editable (priv->entry_view, !future_entry); > + while (affected_actions[i] != NULL) { > > This should be a for-loop: > > for (i = 0; affected_actions[i] != NULL; i++) yes > ::: src/widgets/calendar-button.c > @@ +103,3 @@ > + "Main Window", > "Almanah main window.", > + > GTK_TYPE_APPLICATION_WINDOW, > + > G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); > > This main-window property (and all associated code) could be removed now. yes. > @@ +131,3 @@ > + NULL, NULL, > + NULL, > + G_TYPE_NONE, 0); > > Why does this need to be exposed? Can’t you handle this internally in the > CalendarButton widget and update the widget to select today? At the moment, > this signal is emitted, MainWindow handles it, and then just calls > almanah_calendar_button_select_date(). yes.
Created attachment 296231 [details] [review] App: Migrated GtkUIManager/GtkAction to GAction All the stuff related to GtkUIManager and GActions has been removed to support the modern GAction framework. The actions in data/almana.ui has been added as main window actions, with the "win." prefix, and keyboard accelerators has been moved to the application. The "jump-to-today" action has been removed, because the CalendarButton can change directly the date in the Calendar.
Review of attachment 296231 [details] [review]: Looks good to me from a quick check over. I have not had a chance to test it.
Thanks Philip for take the time to review the patch. I'll commit ASAP.
Attachment 296231 [details] pushed as 33d713c - App: Migrated GtkUIManager/GtkAction to GAction