<p><b>@b4n</b> commented on this pull request.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/664#discussion_r158561907">addons/src/ao_colortip.c</a>:</p>
<pre style='color:#555'>> + * $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))
</pre>
<p>it is recommended nowadays to have a <code>priv</code> 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.<br>
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.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/664#discussion_r158563388">addons/src/ao_colortip.c</a>:</p>
<pre style='color:#555'>> +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));
+
</pre>
<p>shouldn't you disconnect the signal handler (<code>on_editor_button_press_event</code>) here? as you use <code>plugin_signal_connect()</code> 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.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/664#discussion_r158563693">addons/src/ao_colortip.c</a>:</p>
<pre style='color:#555'>> + *
+ * 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$
</pre>
<p>I guess that's a c&p leftover? doesn't seem very useful with Git, is it :)</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/664#discussion_r158564082">addons/src/ao_colortip.c</a>:</p>
<pre style='color:#555'>> +
+
+#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
</pre>
<p>naming convention: normally GObjects names is <code>FooBar</code> and <code>FOO_BAR</code>; so here it would be <code>AoColortip</code> and <code>AO_COLORTIP</code>, or <code>AoColorTip</code> and <code>AO_COLOR_TIP</code>.<br>
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)</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/664#discussion_r158564231">addons/src/ao_colortip.h</a>:</p>
<pre style='color:#555'>> + */
+
+
+#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),\
</pre>
<p>Also about naming, the conventional naming would be <code>AO_IS_COLORTIP</code> (or <code>AO_IS_COLOR_TIP</code>) -- e.g. <code>AO</code> is the namespace.<br>
But anyway, following the convention the plugin already has makes sense.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/664#discussion_r158567521">addons/src/ao_colortip.c</a>:</p>
<pre style='color:#555'>> + 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;
+}
</pre>
<p>any reason why you don't simply use <code>editor_get_word_at_pos()</code>? Maybe something similar to this or whatnot.</p>
<div class="highlight highlight-source-c"><pre>gchar *word = editor_get_word_at_pos(doc->editor, -<span class="pl-c1">1</span>, <span class="pl-s"><span class="pl-pds">"</span>0123456789abcdefABCDEF<span class="pl-pds">"</span></span>);
<span class="pl-k">if</span> (word)
{
gint color = -<span class="pl-c1">1</span>;
<span class="pl-k">switch</span> (<span class="pl-c1">strlen</span> (color))
{
<span class="pl-k">case</span> <span class="pl-c1">3</span>:
color = ((<span class="pl-c1">g_ascii_xdigit_value</span>(word[<span class="pl-c1">0</span>]) * <span class="pl-c1">0x11</span>) << <span class="pl-c1">16</span> |
(<span class="pl-c1">g_ascii_xdigit_value</span>(word[<span class="pl-c1">1</span>]) * <span class="pl-c1">0x11</span>) << <span class="pl-c1">8</span> |
(<span class="pl-c1">g_ascii_xdigit_value</span>(word[<span class="pl-c1">2</span>]) * <span class="pl-c1">0x11</span>) << <span class="pl-c1">0</span>);
<span class="pl-k">break</span>;
<span class="pl-k">case</span> <span class="pl-c1">6</span>:
color = (<span class="pl-c1">g_ascii_xdigit_value</span>(word[<span class="pl-c1">0</span>]) << <span class="pl-c1">20</span> |
<span class="pl-c1">g_ascii_xdigit_value</span>(word[<span class="pl-c1">1</span>]) << <span class="pl-c1">16</span> |
<span class="pl-c1">g_ascii_xdigit_value</span>(word[<span class="pl-c1">2</span>]) << <span class="pl-c1">12</span> |
<span class="pl-c1">g_ascii_xdigit_value</span>(word[<span class="pl-c1">3</span>]) << <span class="pl-c1">8</span> |
<span class="pl-c1">g_ascii_xdigit_value</span>(word[<span class="pl-c1">4</span>]) << <span class="pl-c1">4</span> |
<span class="pl-c1">g_ascii_xdigit_value</span>(word[<span class="pl-c1">5</span>]) << <span class="pl-c1">0</span>);
<span class="pl-k">break</span>;
<span class="pl-k">default</span>:
<span class="pl-c"><span class="pl-c">/*</span> invalid color or other format <span class="pl-c">*/</span></span>
<span class="pl-k">break</span>;
}
<span class="pl-c"><span class="pl-c">//</span> …</span></pre></div>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/geany/geany-plugins/pull/664#pullrequestreview-85400385">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABDrJ3WhGUZEYj7IJenaqMQBsEhL9kJDks5tDDbHgaJpZM4RJpAw">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/ABDrJ06ujakeuDHMLgOX5WfMCkTLfX7nks5tDDbHgaJpZM4RJpAw.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="https://github.com/geany/geany-plugins/pull/664#pullrequestreview-85400385"></link>
<meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/geany/geany-plugins","title":"geany/geany-plugins","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/geany/geany-plugins"}},"updates":{"snippets":[{"icon":"PERSON","message":"@b4n commented on #664"}],"action":{"name":"View Pull Request","url":"https://github.com/geany/geany-plugins/pull/664#pullrequestreview-85400385"}}}</script>