GNOME Bugzilla – Bug 667797
Animate calibration clicks
Last modified: 2013-05-17 14:38:11 UTC
Created attachment 205120 [details] calibration demo As per video
What is the status of this? I assume you have a patch for this which you used in the video. If so, it would be nice if we could integrate it.
It's a video mockup, there's no patch.
Maintainer change
Looking into it. AFAIU, the changes need to the current one are: 1) Window bg is transparent; 2) Text "animates in" under the timeout indicator; 3) Targets show up by zooming-in (with a bit of "elastic go back") and disappear by animating the outer circle towards the center. Probably it'll be much easier to do with Clutter so I might use it instead of (just) cairo.
Created attachment 241077 [details] [review] Patch with the animations Hi, This patch re-writes the calibrator's GUI using Clutter and introduces the animations as faithful to the demo video as I could get. I will also upload a video tomorrow. It needs to be checked with dual-screen and I wonder if it might be better to refactor the calib_area_new and put all the initialization and configuration of stuff in separate functions.
Created attachment 241184 [details] [review] New patch with an embedded Clutter stage I am attaching a new patch which uses an embedded Clutter stage instead of a pure one. This had to be done in order to make the calibrator UI appear on the correct screen and it also allows to keep more consistency between some of the previous behaviors (like quitting it when unfocusing the window). This way we could have possibly maintained the old gui_gtk file name but let's just have the new one because it's better.
Created attachment 241241 [details] [review] Embedded clutter stage and clock actor Hi, This new revision makes the clock a separate Clutter actor after Bastien's suggestion on IRC.
Oh, BTW, I didn't use a private struct member in the Clock actor because what's needed to keep internal is simply a float so I didn't think that was necessary. If it is, let me know and I'll add the private.
Review of attachment 241241 [details] [review]: ::: panels/wacom/calibrator/Makefile.am @@ +30,3 @@ + calibratorgui.c \ + calibratorgui.h \ + clock.c \ Those files aren't in the patch.
Created attachment 241281 [details] [review] New patch with an embedded Clutter stage and forgotten files Argh, sorry about it.
Review of attachment 241281 [details] [review]: Jakub will have to review the animations when he comes back to check that they match the video, to me the animations seemed a little bit fast. It's already better though :) ::: panels/wacom/calibrator/calibratorgui.c @@ +242,3 @@ + +static void +draw_target (ClutterCairoTexture *texture, I think the targets could also do with being split in their own actor. @@ +619,3 @@ + calib_area->calibrator.geometry.height = clutter_actor_get_height (stage); + + if (clutter_color_from_string (&color, "#000")) I don't see any reasons why that should fail, use a g_assert_not_reached() in the failure case instead. You can also use CLUTTER_COLOR_BLACK. @@ +622,3 @@ + { + color.alpha = WINDOW_OPACITY; + clutter_actor_set_background_color (stage, &color); That doesn't seem to work, and the window isn't see-through. @@ +820,3 @@ + gtk_widget_realize (calib_area->window); + window = gtk_widget_get_window (calib_area->window); + gdk_window_set_background_rgba (window, &black); I think you should set the alpha on the background color as well. ::: panels/wacom/calibrator/clock.h @@ +19,3 @@ + */ + +#ifndef __CLOCK_H__ "clock" is too generic. Please rename the object/files/etc. to CcClockActor, or CcCountdownActor. It makes it clear that the object is a ClutterActor, and avoid clashes with system functions.
(In reply to comment #11) > Review of attachment 241281 [details] [review]: > > Jakub will have to review the animations when he comes back to check that they > match the video, to me the animations seemed a little bit fast. It's already > better though :) > > ::: panels/wacom/calibrator/calibratorgui.c > @@ +242,3 @@ > + > +static void > +draw_target (ClutterCairoTexture *texture, > > I think the targets could also do with being split in their own actor. Alright. > > @@ +619,3 @@ > + calib_area->calibrator.geometry.height = clutter_actor_get_height (stage); > + > + if (clutter_color_from_string (&color, "#000")) > > I don't see any reasons why that should fail, use a g_assert_not_reached() in > the failure case instead. > You can also use CLUTTER_COLOR_BLACK. Okay. > > @@ +622,3 @@ > + { > + color.alpha = WINDOW_OPACITY; > + clutter_actor_set_background_color (stage, &color); > > That doesn't seem to work, and the window isn't see-through. I guess this might be related to something else because I couldn't also see the original one as transparent (even though the one from my own machine's g-c-c is). I just assumed this was a problem in my jhbuild... Could you check if the original calibrator code inside jhbuild shows a semi-transparent window for you? > > @@ +820,3 @@ > + gtk_widget_realize (calib_area->window); > + window = gtk_widget_get_window (calib_area->window); > + gdk_window_set_background_rgba (window, &black); > > I think you should set the alpha on the background color as well. This is code from the original version, I didn't touch that but it's probably worth checking if setting the alpha does anything. > > ::: panels/wacom/calibrator/clock.h > @@ +19,3 @@ > + */ > + > +#ifndef __CLOCK_H__ > > "clock" is too generic. > > Please rename the object/files/etc. to CcClockActor, or CcCountdownActor. It > makes it clear that the object is a ClutterActor, and avoid clashes with system > functions. Completely forgot to rename this in the end... Thanks for the comments.
Created attachment 241629 [details] [review] New revision of the patch Hi Bastien, I am attaching a new version of this patch. I tried a lot of solutions for the transparency problem but it seems there's not only a bug in gtk+ but also in clutter-gtk as the bg for the GtkClutterEmbed is always black (I couldn't override it). I'll write the test file for this case and open a bug to the clutter-gtk folks.
There's another issue I just found out. Calling the calibration from g-c-c crashes because Clutter hasn't been initialized. I had initialized it for test-wacom but I forgot about the whole application. I'll provide a fix soon.
Created attachment 241886 [details] [review] New patch which initializes Clutter Hi, as promised, here's a new version of the patch correctly initializing Clutter.
Created attachment 243056 [details] [review] Calibration written in Clutter Hi, I finally found a way to make the transparency work on the window: I had to apply the RGBA screen visual to the window... I also added the deletion of the gui_gtk.c to the patch, which I had forgotten.
Review of attachment 243056 [details] [review]: Nearly ready to go. ::: panels/wacom/calibrator/calibrator-gui.c @@ +231,3 @@ + { + set_success (area); + g_timeout_add(END_TIME, (GSourceFunc) draw_success_end_wait_callback, area); space between function name and bracket. @@ +369,3 @@ + return FALSE; + + /* If the calibrator window looses focus, simply bail out... */ "loses". @@ +537,3 @@ + clutter_text_set_line_alignment (CLUTTER_TEXT (calib_area->helper_text_title), + PANGO_ALIGN_CENTER); + color.red = 127; Please, no magic numbers. @@ +540,3 @@ + color.green = 127; + color.blue = 127; + color.alpha = 255; 0xff? ::: panels/wacom/calibrator/cc-clock-actor.c @@ +25,3 @@ +#define CLOCK_LINE_WIDTH 10 +#define CLOCK_LINE_PADDING 10 +#define ANGLE_PROP_NAME "angle" It's used in a single place, use "angle" directly. @@ +134,3 @@ + gfloat *natural_width_p) +{ + *min_width_p = CLOCK_RADIUS + 2; No magic numbers. @@ +140,3 @@ +static void +cc_clock_actor_get_preferred_height (ClutterActor *actor, + gfloat for_width, Indentation? ::: panels/wacom/calibrator/cc-target-actor.c @@ +106,3 @@ + gpointer data) +{ + cairo_set_source_rgb(cr, 1.0, 1.0, 1.0); Space before brackets all around.
Created attachment 244136 [details] [review] Updated patch Hi, hopefully this new version will address the remaining issues. Cheers,
Created attachment 244500 [details] [review] Rebased patch Same patch, rebased with the latest master.
Review of attachment 244500 [details] [review]: Looks good.
Pushed.