GNOME Bugzilla – Bug 758554
edit-palette: Add a vertical accordion effect when the palette slides in
Last modified: 2015-12-09 14:18:00 UTC
When the EditPalette slides in from the right, the EditTool rows should be revealed with a vertical accordion effect. Look at 0:06 in https://www.youtube.com/watch?v=F3_e24QHzfI
There is an initial implementation from Umang Jain that looks like this: https://www.youtube.com/watch?v=FhWs4zQO2wQ
Created attachment 316473 [details] [review] Accordian Effect
Review of attachment 316473 [details] [review]: Thanks for the patch, Umang. A few quick comments: ::: src/photos-edit-palette-row.c @@ +81,3 @@ + GTK_REVEALER_TRANSITION_TYPE_SLIDE_DOWN); + gtk_revealer_set_transition_duration (GTK_REVEALER (self->row_revealer), 500 + (row_no * 100)); + gtk_revealer_set_reveal_child (GTK_REVEALER (self->row_revealer), FALSE); The default value of GtkRevealer:reveal-child is FALSE. So there shouldn't be any need to set it to FALSE. ::: src/photos-edit-palette-row.h @@ +51,3 @@ void photos_edit_palette_row_show_details (PhotosEditPaletteRow *self); +void photos_edit_palette_show_accordian (PhotosEditPaletteRow *self); photos_edit_palette_row_show, would be a better name because: (i) The prefix of public symbols should first have the global namespace (ie. photos/Photos), and then the class name (ie. edit_palette_row/EditPaletteRow). That is how namespacing is done in C. (ii) The accordion effect is an internal behavioural detail of the rows. It is not hard to imagine that we might change it to something slightly different in future. It's better not to leak internal details in public API. Also, please fix the alignment. ::: src/photos-edit-palette.c @@ +199,3 @@ + for (i = 0; (row = gtk_list_box_get_row_at_index (GTK_LIST_BOX (self), i)) != NULL; i++) + photos_edit_palette_show_accordian (PHOTOS_EDIT_PALETTE_ROW (row)); +} I think this should just be a part of photos_edit_palette_show.
Review of attachment 316473 [details] [review]: ::: src/photos-edit-palette-row.c @@ +65,3 @@ + + if (row_no % 4 == 0) + row_no = 0; /* RESET row_no if four rows are constructed, used in accordian effect revelar */ Instead of doing this, set the transition duration of each row in EditPalette.
Created attachment 316728 [details] [review] Accordian Effect
Accordion effect Video casts with two different implementations: 1) gtk_revealer_set_transition_duration (step_timer_for_each_row) - https://youtu.be/hQ8z0-kVtOQ 2)gtk_revealer_set_transition_duration (same_timer_for_all_rows) - https://youtu.be/Lir3oiacUxo 3) To understand better the effect #2 - https://youtu.be/T-yacoHGnrc (with 5 sec gtk_revealer_set_transition_duration (5 seconds) Currently, I have implemented with #2, with gtk_revealer_set_transition_duration using its default value i.e. gtk_revealer_set_transition_duration is *NOT* explicitly mentioned in the code, therefore the row_revealer is using its default transition duration value.
Review of attachment 316728 [details] [review]: This looks much better. ::: src/photos-edit-palette-row.c @@ +75,3 @@ + self->row_revealer = gtk_revealer_new(); + gtk_revealer_set_transition_type (GTK_REVEALER (self->row_revealer), + GTK_REVEALER_TRANSITION_TYPE_SLIDE_DOWN); Broken alignment. Could have been in the same line. @@ +77,3 @@ + GTK_REVEALER_TRANSITION_TYPE_SLIDE_DOWN); + gtk_container_add (GTK_CONTAINER (self), self->row_revealer); + gtk_container_add (GTK_CONTAINER (self->row_revealer), grid0); Please create the row_reveler before grid0. I find that order more readable. @@ +227,3 @@ gtk_revealer_set_reveal_child (GTK_REVEALER (self->details_revealer), TRUE); } + 2 lines between functions, not 1. ::: src/photos-edit-palette.c @@ +1,3 @@ /* * Photos - access, organize and share your photos on GNOME + * Copyright © Umang Jain We generally update this when the modifications are significant. With the following change, it will just be one function call, which doesn't count. @@ +232,3 @@ + for (i = 0; (edit_palette_row = gtk_list_box_get_row_at_index (GTK_LIST_BOX (self), i)) != NULL; i++) + photos_edit_palette_row_show (PHOTOS_EDIT_PALETTE_ROW (edit_palette_row)); Instead of a separate for loop, we can just call photos_edit_palette_row_show in the previous loop where we are creating the rows.
Created attachment 317037 [details] [review] edit-palette, edit-palette-row, Add a vertical accordian effect I fixed up the patch and pushed it.
Thanks for your effort, Umang!