On Tue, Jun 21, 2016 at 11:50 PM, Eduardo Lima (Etrunko)
<etrunko@xxxxxxxxxx> wrote:
> On 06/21/2016 05:04 PM, Fabiano Fidêncio wrote:
>
>> diff --git a/src/view/virt-viewer-timed-revealer.c b/src/view/virt-viewer-timed-revealer.c
>> new file mode 100644
>> index 0000000..bba363c
>> --- /dev/null
>> +++ b/src/view/virt-viewer-timed-revealer.c
>> @@ -0,0 +1,224 @@
>> +/*
>> + * Virt Viewer: A virtual machine console viewer
>> + *
>> + * Copyright (c) 2016 Red Hat, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> + *
>> + * Author: Cole Robinson <crobinso@xxxxxxxxxx>
>> + * Author: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
>> + */
>> +
>> +#include <config.h>
>> +
>> +#include "virt-viewer-timed-revealer.h"
>> +
>> +G_DEFINE_TYPE (VirtViewerTimedRevealer, virt_viewer_timed_revealer, G_TYPE_OBJECT)
>> +
>> +#define VIRT_VIEWER_TIMED_REVEALER_GET_PRIVATE(obj) \
>> + (G_TYPE_INSTANCE_GET_PRIVATE ((obj), VIRT_VIEWER_TYPE_TIMED_REVEALER, VirtViewerTimedRevealerPrivate))
>> +
>> +struct _VirtViewerTimedRevealerPrivate
>> +{
>> + gboolean fullscreen;
>> + guint timeout_id;
>> +
>> + GtkWidget *revealer;
>> + GtkWidget *evBox;
>> +};
>> +
>> +static void
>> +virt_viewer_timed_revealer_unregister_timeout(VirtViewerTimedRevealer *self)
>> +{
>> + VirtViewerTimedRevealerPrivate *priv;
>> +
>> + priv = self->priv;
>> +
>
> I can see you don't like using self->priv->, so maybe you could just
> initialize priv with the declaration on the line above?
Yeah, changed them all for the v2.
>
>> + if (priv->timeout_id) {
>> + g_source_remove(priv->timeout_id);
>> + priv->timeout_id = 0;
>> + }
>> +}
>> +
>> +static gboolean
>> +schedule_unreveal_timeout_cb(VirtViewerTimedRevealer *self)
>> +{
>> + VirtViewerTimedRevealerPrivate *priv;
>> +
>> + priv = self->priv;
>> +
>
> Here too.
>
>> + gtk_revealer_set_reveal_child(GTK_REVEALER(priv->revealer), FALSE);
>> + priv->timeout_id = 0;
>> +
>> + return FALSE;
>> +}
>> +
>> +static void
>> +virt_viewer_timed_revealer_schedule_unreveal_timeout(VirtViewerTimedRevealer *self,
>> + guint timeout)
>> +{
>> + VirtViewerTimedRevealerPrivate *priv;
>> +
>> + priv = self->priv;
>> +
>
> And here.
>
>> + if (priv->timeout_id != 0)
>> + return;
>> +
>> + priv->timeout_id = g_timeout_add(timeout,
>> + (GSourceFunc)schedule_unreveal_timeout_cb,
>> + self);
>> +}
>> +
>> +static gboolean
>> +virt_viewer_timed_revealer_enter_leave_notify(GtkWidget *evBox G_GNUC_UNUSED,
>> + GdkEventCrossing *event G_GNUC_UNUSED,
>
> event is marked G_GNC_UNUSED, but it is used on the function in a couple
> of places.
Yeah. I was using a deprecated method before and when switched to the
code how it is I forgot removing the G_GNUC_UNUSED macro.
Anyways, I can remove also from the evBox and use it instead of using
the one stored in the priv..
>
>> + VirtViewerTimedRevealer *self)
>> +{
>> + VirtViewerTimedRevealerPrivate *priv;
>> + GdkDevice *device;
>> + GtkAllocation allocation;
>> + gint x, y;
>> + gboolean entered;
>> +
>> + priv = self->priv;
>
> Same about initializing.
>
>> +
>> + device = gdk_event_get_device((GdkEvent *)event);
>> +
>> + gdk_window_get_device_position(event->window, device, &x, &y, 0);
>> + gtk_widget_get_allocation(priv->evBox, &allocation);
>> +
>> + entered = !!(x >= 0 && y >= 0 && x < allocation.width && y < allocation.height);
>> +
>> + if (!priv->fullscreen)
>> + return FALSE;
>> +
>
> You should test for fullscreen the first thing in the function.
True. Will be fixed in v2.
>
>> + /*
>> + * Pointer exited the toolbar, and toolbar is revealed. Schedule
>> + * a timeout to close it, if one isn't already scheduled.
>> + */
>> + if (!entered && gtk_revealer_get_reveal_child(GTK_REVEALER(priv->revealer))) {
>> + virt_viewer_timed_revealer_schedule_unreveal_timeout(self, 1000);
>> + return FALSE;
>> + }
>> +
>> + virt_viewer_timed_revealer_unregister_timeout(self);
>> + if (entered && !gtk_revealer_get_reveal_child(GTK_REVEALER(priv->revealer))) {
>> + gtk_revealer_set_reveal_child(GTK_REVEALER(priv->revealer), TRUE);
>> + }
>> +
>> + return FALSE;
>> +}
>> +
>> +static void
>> +virt_viewer_timed_revealer_init(VirtViewerTimedRevealer *self)
>> +{
>> + self->priv = VIRT_VIEWER_TIMED_REVEALER_GET_PRIVATE(self);
>> +}
>> +
>> +static void
>> +virt_viewer_timed_revealer_dispose(GObject *object)
>> +{
>> + VirtViewerTimedRevealer *self;
>> + VirtViewerTimedRevealerPrivate *priv;
>> +
>> + self = VIRT_VIEWER_TIMED_REVEALER(object);
>> + priv = self->priv;
>> +
>> + g_clear_object(&priv->evBox);
>> + g_clear_object(&priv->revealer);
>> +
>> + if (priv->timeout_id) {
>> + g_source_remove(priv->timeout_id);
>> + priv->timeout_id = 0;
>> + }
>> +
>> + G_OBJECT_CLASS(virt_viewer_timed_revealer_parent_class)->dispose(object);
>> +}
>> +
>> +
>> +static void
>> +virt_viewer_timed_revealer_class_init(VirtViewerTimedRevealerClass *klass)
>> +{
>> + GObjectClass *object_class = G_OBJECT_CLASS(klass);
>> +
>> + g_type_class_add_private (klass, sizeof (VirtViewerTimedRevealerPrivate));
>> +
>> + object_class->dispose = virt_viewer_timed_revealer_dispose;
>> +}
>> +
>> +VirtViewerTimedRevealer *
>> +virt_viewer_timed_revealer_new(GtkWidget *toolbar)
>> +{
>> + VirtViewerTimedRevealer *self;
>> + VirtViewerTimedRevealerPrivate *priv;
>> +
>> + self = g_object_new(VIRT_VIEWER_TYPE_TIMED_REVEALER, NULL);
>> +
>> + priv = self->priv;
>> +
>> + priv->fullscreen = FALSE;
>> + priv->timeout_id = 0;
>> +
>> + priv->revealer = gtk_revealer_new();
>> + gtk_container_add(GTK_CONTAINER(priv->revealer), toolbar);
>> +
>> + /*
>> + * Adding the revealer to the eventbox seems to ensure the
>> + * GtkEventBox always has 1 invisible pixel showing at the top of the
>> + * screen, which we can use to grab the pointer event to show
>> + * the hidden toolbar.
>> + */
>> +
>> + priv->evBox = gtk_event_box_new();
>> + gtk_container_add(GTK_CONTAINER(priv->evBox), priv->revealer);
>> + gtk_widget_set_halign(priv->evBox, GTK_ALIGN_CENTER);
>> + gtk_widget_set_valign(priv->evBox, GTK_ALIGN_START);
>> + gtk_widget_show_all(priv->evBox);
>> +
>> + g_signal_connect(priv->evBox,
>> + "enter-notify-event",
>> + G_CALLBACK(virt_viewer_timed_revealer_enter_leave_notify),
>> + self);
>> + g_signal_connect(priv->evBox,
>> + "leave-notify-event",
>> + G_CALLBACK(virt_viewer_timed_revealer_enter_leave_notify),
>> + self);
>> +
>> + return self;
>> +}
>> +
>> +void
>> +virt_viewer_timed_revealer_force_reveal(VirtViewerTimedRevealer *self,
>> + gboolean fullscreen)
>> +{
>> + VirtViewerTimedRevealerPrivate *priv;
>> +
>> + g_return_if_fail(VIRT_VIEWER_IS_TIMED_REVEALER(self));
>> +
>
> I think you want to play safe here, but is there any chance this check
> fails?
Only if the caller of this function fouls up :-)
I'd rather keep this check and the one in function below.
>
>> + priv = self->priv;
>> +
>> + virt_viewer_timed_revealer_unregister_timeout(self);
>> + priv->fullscreen = fullscreen;
>> + gtk_revealer_set_reveal_child(GTK_REVEALER(priv->revealer), fullscreen);
>> + virt_viewer_timed_revealer_schedule_unreveal_timeout(self, 2000);
>> +}
>> +
>> +GtkWidget *
>> +virt_viewer_timed_revealer_get_overlay_widget(VirtViewerTimedRevealer *self)
>> +{
>> + g_return_val_if_fail(VIRT_VIEWER_IS_TIMED_REVEALER(self), NULL);
>> +
>> + return self->priv->evBox;
>
> BUSTED!!!! self->priv-> usage spotted. :D
:-) Yeah, and I'd like to keep this one as it is.
Not only to prove that I'm not consistent, but also because I don't
think would worth to add the Private here and ended up spending more
lines/chars than saving them.
>
>> +}
>
>
> --
> Eduardo de Barros Lima (Etrunko)
> Software Engineer - RedHat
> etrunko@xxxxxxxxxx
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/virt-tools-list
Thanks for the review,
--
Fabiano Fidêncio
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
