After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 758554 - edit-palette: Add a vertical accordion effect when the palette slides in
edit-palette: Add a vertical accordion effect when the palette slides in
Status: RESOLVED FIXED
Product: gnome-photos
Classification: Applications
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: GNOME photos maintainer(s)
GNOME photos maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-11-23 20:06 UTC by Debarshi Ray
Modified: 2015-12-09 14:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Accordian Effect (4.94 KB, patch)
2015-11-29 16:35 UTC, Umang Jain
none Details | Review
Accordian Effect (3.92 KB, patch)
2015-12-03 16:59 UTC, Umang Jain
committed Details | Review
edit-palette, edit-palette-row, Add a vertical accordian effect (3.78 KB, patch)
2015-12-09 14:17 UTC, Debarshi Ray
committed Details | Review

Description Debarshi Ray 2015-11-23 20:06:36 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
Comment 1 Debarshi Ray 2015-11-23 20:08:14 UTC
There is an initial implementation from Umang Jain that looks like this:
  https://www.youtube.com/watch?v=FhWs4zQO2wQ
Comment 2 Umang Jain 2015-11-29 16:35:01 UTC
Created attachment 316473 [details] [review]
Accordian Effect
Comment 3 Debarshi Ray 2015-11-30 10:20:49 UTC
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.
Comment 4 Debarshi Ray 2015-11-30 14:46:48 UTC
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.
Comment 5 Umang Jain 2015-12-03 16:59:12 UTC
Created attachment 316728 [details] [review]
Accordian Effect
Comment 6 Umang Jain 2015-12-03 17:05:46 UTC
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.
Comment 7 Debarshi Ray 2015-12-09 14:09:08 UTC
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.
Comment 8 Debarshi Ray 2015-12-09 14:17:37 UTC
Created attachment 317037 [details] [review]
edit-palette, edit-palette-row, Add a vertical accordian effect

I fixed up the patch and pushed it.
Comment 9 Debarshi Ray 2015-12-09 14:18:00 UTC
Thanks for your effort, Umang!