Closed #1221.
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1221#event-8441754664
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1221/issue_event/8441754664(a)github.com>
>
> Looks good! Thanks.
>
> > While "rainbow colours" sounds nice "Bracketcolors" is more descriptive, so a user looking for it is more likely to find it.
>
> Full agree. @asifamin13 maybe you could just mention "rainbow colors" in the README, that might already suffice that users can find it also with this name.
>
> I did a quick code review without having a deeper look at the logic. From a quick test, it works as advertised and I got many fancy brackets :).
>
> A few comments @asifamin13:
>
> * Could you add the plugin name in https://github.com/geany/geany-plugins/blob/master/build/geany-plugins.nsi#… (this is for the Windows installer, use `bracketcolors.dll`)
> * As Geany itself also colorizes the brackets in an idle callback, did you check that both implementations do not interfere with each other?
> * I got a few compiler warnings which might be worth looking into:
>
> ```
> BracketMap.cc: In member function 'void BracketMap::Show()':
> BracketMap.cc:122:21: warning: loop variable 'it' creates a copy from type 'const std::pair<const int, std::tuple<int, int> >' [-Wrange-loop-construct]
> 122 | for (const auto it : mBracketMap) {
> | ^~
> BracketMap.cc:122:21: note: use reference type to prevent copying
> 122 | for (const auto it : mBracketMap) {
> | ^~
> | &
> bracketcolors.cc: In function 'gboolean utils_parse_color(const gchar*, GdkColor*)':
> bracketcolors.cc:276:27: warning: 'gboolean gdk_color_parse(const gchar*, GdkColor*)' is deprecated: Use 'gdk_rgba_parse' instead [-Wdeprecated-declarations]
> 276 | return gdk_color_parse(spec, color);
> | ~~~~~~~~~~~~~~~^~~~~~~~~~~~~
> In file included from /usr/include/gtk-3.0/gdk/gdkcairo.h:26,
> from /usr/include/gtk-3.0/gdk/gdk.h:33,
> from /usr/include/gtk-3.0/gtk/gtk.h:30,
> from /home/enrico/apps/include/geany/gtkcompat.h:30,
> from /home/enrico/apps/include/geany/editor.h:27,
> from /home/enrico/apps/include/geany/document.h:31,
> from /home/enrico/apps/include/geany/build.h:26,
> from /home/enrico/apps/include/geany/geanyplugin.h:36,
> from bracketcolors.cc:40:
> /usr/include/gtk-3.0/gdk/deprecated/gdkcolor.h:79:11: note: declared here
> 79 | gboolean gdk_color_parse (const gchar *spec,
> | ^~~~~~~~~~~~~~~
> ```
Thanks for the feedback!
- The deprecated `gdk*` warning are ok. I removed the unused debugging code that was causing the others.
- I added the bracketcolors entry to `geany-plugins.nsi`.
- I confirmed my plugin behaves correctly when Geany colors brackets itself, like when brackets are missing
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1221#issuecomment-1418273658
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1221/c1418273658(a)github.com>
@asifamin13 commented on this pull request.
> @@ -0,0 +1,32 @@
+Color brackets, parenthesis, and braces
+===================
thanks, fixed!
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1221#discussion_r1096813044
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1221/review/1284356350(a)github.com>
@asifamin13 commented on this pull request.
> + // nested bracket
+ orderStack.push(endPos);
+ }
+
+ GetOrder(bracket) = orderStack.size() - 1;
+ }
+}
+
+
+
+// -----------------------------------------------------------------------------
+ void BracketMap::Show()
+/*
+
+----------------------------------------------------------------------------- */
+{ g_debug("%s: Showing bracket map ...", __FUNCTION__);
they aren't needed anymore. I removed the commented ones as well
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1221#discussion_r1096812976
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1221/review/1284356259(a)github.com>
@asifamin13 commented on this pull request.
> @@ -0,0 +1,35 @@
+# =============================================================================
yes, is there a better way to handle this? I'm new to the autotools world, so any recommendations would be appreciated
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1221#discussion_r1096812891
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1221/review/1284356188(a)github.com>
@asifamin13 commented on this pull request.
> +
+ static Length& GetLength(Bracket &bracket) {
+ return std::get<0>(bracket);
+ }
+ static Order& GetOrder(Bracket &bracket) {
+ return std::get<1>(bracket);
+ }
+
+ static const Length& GetLength(const Bracket &bracket) {
+ return std::get<0>(bracket);
+ }
+ static const Order& GetOrder(const Bracket &bracket) {
+ return std::get<1>(bracket);
+ }
+};
+
this was from a standard template I use, fixed
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1221#discussion_r1096812788
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1221/review/1284356082(a)github.com>
@asifamin13 commented on this pull request.
> @@ -0,0 +1,143 @@
+/*
+ * BracketMap.cc
+ *
+ * Copyright 2013 Asif Amin <asifamin(a)utexas.edu>
thanks for catching this, fixed!
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1221#discussion_r1096812623
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1221/review/1284355920(a)github.com>
@asifamin13 pushed 1 commit.
0ac738a0532a8638bbe9d9bd55aed0eac671df78 Bracket Colors, initial code review
--
View it on GitHub:
https://github.com/geany/geany-plugins/pull/1221/files/b75167c3c1d56d476abc…
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1221/push/12516732357(a)github.com>
@eht16 commented on this pull request.
> + // nested bracket
+ orderStack.push(endPos);
+ }
+
+ GetOrder(bracket) = orderStack.size() - 1;
+ }
+}
+
+
+
+// -----------------------------------------------------------------------------
+ void BracketMap::Show()
+/*
+
+----------------------------------------------------------------------------- */
+{ g_debug("%s: Showing bracket map ...", __FUNCTION__);
Should those `g_debug` statements remain in the code? Also the commented ones?
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1221#pullrequestreview-12842564…
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1221/review/1284256497(a)github.com>
@eht16 requested changes on this pull request.
Oh, and could you please add your plugin to the list in the top README?
> @@ -0,0 +1,32 @@
+Color brackets, parenthesis, and braces
+===================
RestructuredText (the text format used here) requires that underlines are exactly as long as the heading above.
> @@ -0,0 +1,143 @@
+/*
+ * BracketMap.cc
+ *
+ * Copyright 2013 Asif Amin <asifamin(a)utexas.edu>
2013? I guess it's a typo :).
If so, ideally change all occurrences.
> @@ -0,0 +1,35 @@
+# =============================================================================
Are these two "ax_cxx_*" M4 macros copied from Geany?
> +
+ static Length& GetLength(Bracket &bracket) {
+ return std::get<0>(bracket);
+ }
+ static Order& GetOrder(Bracket &bracket) {
+ return std::get<1>(bracket);
+ }
+
+ static const Length& GetLength(const Bracket &bracket) {
+ return std::get<0>(bracket);
+ }
+ static const Order& GetOrder(const Bracket &bracket) {
+ return std::get<1>(bracket);
+ }
+};
+
Why so many empty lines?
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1221#pullrequestreview-12842505…
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1221/review/1284250581(a)github.com>