[Github-comments] [geany/geany-plugins] addons: show color tip and start Color Chooser with double click (#664)

[Colomban Wendling]

b4n commented on this pull request.



> + * $Id$
+ */
+
+
+#include <gtk/gtk.h>
+#include <glib-object.h>
+
+#include "geanyplugin.h"
+
+#include "addons.h"
+#include "ao_colortip.h"
+
+typedef struct _AoColorTipPrivate			AoColorTipPrivate;
+
+#define AO_COLORTIP_GET_PRIVATE(obj)		(G_TYPE_INSTANCE_GET_PRIVATE((obj),\
+			AO_COLORTIP_TYPE, AoColorTipPrivate))

it is recommended nowadays to have a `priv` member in the object struct that points to the private structure because it's faster. It might not be very important here, but that's a good practice.
But having a private struct is not very useful here at all, as the "public" struct itself is also private anyway, so actually I'd rather get rid of it and but everything in the regular struct.

> +static void connect_documents_button_press_signal_handler(AoColorTip *colortip)
+{
+	guint i = 0;
+	/* connect the button-press event for all open documents */
+	foreach_document(i)
+	{
+		connect_document_button_press_signal_handler(colortip, documents[i]);
+	}
+}
+
+
+static void ao_color_tip_finalize(GObject *object)
+{
+	g_return_if_fail(object != NULL);
+	g_return_if_fail(IS_AO_COLORTIP(object));
+

shouldn't you disconnect the signal handler (`on_editor_button_press_event`) here?  as you use `plugin_signal_connect()` it'll be disconnected when the plugin is unloaded, but you still have to take care it's not called after the AoColortip is destroyed.

> + *
+ *      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., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ *
+ * $Id$

I guess that's a c&p leftover?  doesn't seem very useful with Git, is it :)

> +
+
+#include <gtk/gtk.h>
+#include <glib-object.h>
+
+#include "geanyplugin.h"
+
+#include "addons.h"
+#include "ao_colortip.h"
+
+typedef struct _AoColorTipPrivate			AoColorTipPrivate;
+
+#define AO_COLORTIP_GET_PRIVATE(obj)		(G_TYPE_INSTANCE_GET_PRIVATE((obj),\
+			AO_COLORTIP_TYPE, AoColorTipPrivate))
+
+struct _AoColorTip

naming convention: normally GObjects names is `FooBar` and `FOO_BAR`; so here it would be `AoColortip` and `AO_COLORTIP`, or `AoColorTip` and `AO_COLOR_TIP`.
That's more a FYI comment, it doesn't actually matter here, it's only rather important to follow if you use automated tools relying on these conventions (like possibly API extractors & co)

> + */
+
+
+#ifndef __AO_COLORTIP_H__
+#define __AO_COLORTIP_H__
+
+G_BEGIN_DECLS
+
+#define AO_COLORTIP_TYPE				(ao_color_tip_get_type())
+#define AO_COLORTIP(obj)				(G_TYPE_CHECK_INSTANCE_CAST((obj),\
+			AO_COLORTIP_TYPE, AoColorTip))
+#define AO_COLORTIP_CLASS(klass)		(G_TYPE_CHECK_CLASS_CAST((klass),\
+			AO_COLORTIP_TYPE, AoColorTipClass))
+#define IS_AO_COLORTIP(obj)				(G_TYPE_CHECK_INSTANCE_TYPE((obj),\
+			AO_COLORTIP_TYPE))
+#define IS_AO_COLORTIP_CLASS(klass)		(G_TYPE_CHECK_CLASS_TYPE((klass),\

Also about naming, the conventional naming would be `AO_IS_COLORTIP` (or `AO_IS_COLOR_TIP`) -- e.g. `AO` is the namespace.
But anyway, following the convention the plugin already has makes sense.

> +	if (end > max)
+	{
+		end = max;
+	}
+
+	/* Get text in range and examine it */
+	subtext = sci_get_contents_range(doc->editor->sci, start, end);
+	if (subtext != NULL)
+	{
+		pos = pos - start;
+		color = contains_color_value(subtext, pos, 1);
+		g_free(subtext);
+	}
+
+	return color;
+}

any reason why you don't simply use `editor_get_word_at_pos()`?  Maybe something similar to this or whatnot.

```c
gchar *word = editor_get_word_at_pos(doc->editor, -1, "0123456789abcdefABCDEF");

if (word)
{
    gint color = -1;

    switch (strlen (color))
    {
        case 3:
            color = ((g_ascii_xdigit_value(word[0]) * 0x11) << 16 |
                     (g_ascii_xdigit_value(word[1]) * 0x11) << 8 |
                     (g_ascii_xdigit_value(word[2]) * 0x11) << 0);
            break;
        case 6:
            color = (g_ascii_xdigit_value(word[0]) << 20 |
                     g_ascii_xdigit_value(word[1]) << 16 |
                     g_ascii_xdigit_value(word[2]) << 12 |
                     g_ascii_xdigit_value(word[3]) << 8 |
                     g_ascii_xdigit_value(word[4]) << 4 |
                     g_ascii_xdigit_value(word[5]) << 0);
            break;
        default:
            /* invalid color or other format */
            break;
    }
    // …
````

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/664#pullrequestreview-85400385
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20171222/5df9d26f/attachment.html>