Add bracket colors plugin #1221

asifamin13 · 2023-01-31T00:57:42Z

Bracketcolors plugin

Color { }, [ ], ( ) based on nesting order to make it easier to see the start/end bracket
Ignore brackets in non source code (strings, comments, doc strings, etc.)
Language agnostic

How it works

Allocates 3 indicators (starting at INDICATOR_IME - 3) to color each bracket pair as determined by SCI_BRACEMATCH. Ignore non source brackets as determined by highlighting_is_code_style

Dependencies

Geany 1.38
- Need INDICATOR_IME from updated scintilla release
C++17
- Mainly for std::map

Demo

rdipardo · 2023-02-04T20:01:30Z

Perhaps the description and/or commit message should mention #1148?

asifamin13 · 2023-02-04T21:51:59Z

I updated the commit message with a reference to that issue. Raindow brackets sounds cool, I wish I knew of that issue before starting this

elextr · 2023-02-04T23:32:47Z

While "rainbow colours" sounds nice "Bracketcolors" is more descriptive, so a user looking for it is more likely to find it.

eht16 · 2023-02-05T15:55:32Z

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#L176 (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,
      |           ^~~~~~~~~~~~~~~

eht16

Oh, and could you please add your plugin to the list in the top README?

bracketcolors/README

bracketcolors/src/BracketMap.cc

build/ax_cxx_compile_stdcxx_17.m4

bracketcolors/src/BracketMap.h

bracketcolors/src/BracketMap.cc

asifamin13 · 2023-02-05T21:49:44Z

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#L176 (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

asifamin13 · 2023-02-05T22:01:38Z

@eht16 I incorporated your suggested changes. I'm still learning Github, I apologize if I'm not navigating this pull request correctly 😅 (at my work, I'm used to Gitlab)

eht16 · 2023-02-12T15:04:36Z

@eht16 I incorporated your suggested changes. I'm still learning Github, I apologize if I'm not navigating this pull request correctly sweat_smile (at my work, I'm used to Gitlab)

No worries, it's fine.
Thanks for the changes. The PR looks good to me and I would merge it in a few days if nobody objects.

b4n · 2023-03-09T20:53:07Z

bracketcolors/src/bracketcolors.cc

+    static gchar char_at(ScintillaObject *sci, gint pos)
+/*
+
+----------------------------------------------------------------------------- */
+{
+    return sci_get_char_at(sci, pos);
+}


Just curious: why the wrapper?

Just curious: why the wrapper?

The documented return type of SCI_GETCHARAT is int, presumably for Unicode characters up to 32 bits in length. Squeezing that into an 8-bit(?) (g)char needs an explicit cast.

The (implied) point that just using the returned (g)int without a cast is worth considering. At the same time, the pos argument might be safer as the (platform independent) Sci_Position type. The API structures that can only index 32-bit positions are slated for eventual deprecation.

The documented return type of SCI_GETCHARAT is int, presumably for Unicode characters up to 32 bits in length.

No, that's probably either because int is "the" basic numerical type in C, or because of other historical reasons (and technically it is returned as a sptr_t). In practice, the returned value comes from Document::ChatAt() which returns a char. So if if want to be picky, it's actually a char stored into a sptr_t.

Squeezing that into an 8-bit(?) (g)char needs an explicit cast.

Well, yes, but… actually no: this does not actually wrap SCI_GETCHARAT, but Geany's own wrapper, sci_get_char_at() which has the exact same signature.

When I was writing this, I was using the autoclose plugin as a template which has this wrapper as well. I suspect the authors of autoclose used the wrapper for convenience since it's used a lot, I don't really need it as bad to be honest.

elextr · 2023-03-11T00:33:41Z

? why removed

rdipardo · 2023-03-12T01:01:31Z

? why removed

To get your attention, it seems.

elextr · 2023-03-12T01:27:55Z

Nah, it happens that people learning Git can stuff it up so bad its easier to start again, no problem.

But unfortunately that disconnects the discussion.

b4n · 2023-03-12T12:55:52Z

Instead of breaking the flow, just force-push your branch: git push -f origin bracketcolors_v1. You probably can even revive this one by simply reopening it, and if GitHub isn't smart enough, there's a bunch of tricks we could do to force it's hand.

b4n · 2023-03-12T12:56:50Z

Nah, it happens that people learning Git can stuff it up so bad its easier to start again, no problem.

Well, it usually is not easier, but people just don't know it 😅

elextr · 2023-03-13T00:40:40Z

Nah, it happens that people learning Git can stuff it up so bad its easier to start again, no problem.

Well, it usually is not easier, but people just don't know it 😅

Thats learning 😁

asifamin13 · 2023-03-14T04:02:18Z

Instead of breaking the flow, just force-push your branch: git push -f origin bracketcolors_v1. You probably can even revive this one by simply reopening it, and if GitHub isn't smart enough, there's a bunch of tricks we could do to force it's hand.

This is what I did, along with squashing several commits similar to this. I made the mistake of forgetting that I pressed the "sync with upstream" button (due to a warning I saw on the UI), and I ended up including (and taking ownership) of the upstream commits. Since the commits were all squashed together, I couldn't see a way to cleanly git revert. I then panicked and renamed that branch 'bracketcolors_v1_broken' and started over again with a new 'bracketcolors_v1' branch where I cherry picked the relevant commits I cared about from an older tree. But that ended up closing this pull request 💀

If any of yall can somehow reverse that mess, I owe you a beer.

b4n · 2023-03-14T08:39:28Z

If any of yall can somehow reverse that mess, I owe you a beer.

Challenge accepted, I'll see if I can convince GitHub quickly 🍺
Otherwise don't worry, it's not the end of the world

b4n · 2023-03-14T08:48:41Z

@asifamin13 actually I can't do it because I don't have permission to push to your branch.

If you want to try it out, I would think you'd have to do this:

Close Add bracket colors plugin #1241 (GitHub doesn't like 2 PRs referring to the same branch)
Recover this PR with git push -f asifamin13 1e9a8b0c32d15c27ba448e689545c0bc5c9b97aa:bracketcolors_v1 (assuming the remote to your own fork is asifamin13)
Reopen this PR
Re-add your latest changes: git push -f asifamin13 bracketcolors_v1.

Do not pull in between the steps, as step 2 is restoring your old state temporarily for GitHub's sake, and step 4 is trying to re-put your new changes there.

But again, if you're feeling out of your depth, there are two options:

allow maintainers to push to your PR
don't worry and accept that it's the cost of learning

asifamin13 · 2023-03-15T03:14:32Z

@b4n I added you as a collaborator, let me know if there are any problems.

I had some trouble with step 2

fatal: bad object 1e9a8b0c32d15c27ba448e689545c0bc5c9b97aa
fatal: the remote end hung up unexpectedly

I'm a bit out of my depth but I'm willing to learn. Where does that hash come from?

b4n · 2023-03-15T09:16:32Z

@b4n I added you as a collaborator, let me know if there are any problems.

Here you go, that PR is restored and should match what you had in #1241, 🍻
You can now push whatever you need (including force-pushing if necessary) and it should properly update this PR.

I had some trouble with step 2
fatal: bad object 1e9a8b0c32d15c27ba448e689545c0bc5c9b97aa
fatal: the remote end hung up unexpectedly
I'm a bit out of my depth but I'm willing to learn. Where does that hash come from?

1e9a8b0 was the commit this PR was at (as seen in the commits history before I force-pushed, and as you can see in the force-push notification above). The idea was to restore the branch to the state it had when this PR was closed so GitHub sees it as matching this PR, and accept re-opening this PR. If you didn't have that hash anymore, it means that Git's garbage collection got rid of it on your end (as it was an old commit not referenced by anything anymore); you could have gotten it back by fetching it from GitHub (as I did myself, git fetch asifamin13 1e9a8b0c32d15c27ba448e689545c0bc5c9b97aa).

asifamin13 · 2023-03-20T03:56:53Z

@b4n you are a life saver, your next beer is on me 🍻

I didn't realize git had garbage collection, I'll be sure to git fetch next time

This PR is back on track now 😄

asifamin13 · 2023-04-06T17:56:26Z

Fixed a bug to address a case when nested brackets weren't getting their color updated when their parent gets updated

rdipardo · 2023-04-07T02:35:07Z

@asifamin13,

Fixed a bug to address a case when nested brackets weren't getting their color updated when their parent gets updated

Looks like a good improvement.

In the future, though, I would concentrate on just getting a minimal working version merged, then touch it up later in small, traceable iterations. To paraphrase the famous saying of Valéry, a pull request is never finished; it's abandoned.

elextr · 2023-04-07T02:47:46Z

To paraphrase the famous saying of Valéry, a pull request is never finished; it's abandoned.

Wise(ish) words, and its corollary, 'Every time a PR changes "somebody" has to re-check it and re-test it, and eventually "somebody" gets bored' :-)

asifamin13 · 2023-04-18T16:57:38Z

In the future, though, I would concentrate on just getting a minimal working version merged, then touch it up later in small, traceable iterations. To paraphrase the famous saying of Valéry, a pull request is never finished; it's abandoned.

Understood. What it currently on this branch is the "minimal working version". There is one more feature I want to add (user defined colors via preferences menu/config file). I'm working on this now, hopefully to be finished this week. I'll keep that on a different commit

Wise(ish) words, and its corollary, 'Every time a PR changes "somebody" has to re-check it and re-test it, and eventually "somebody" gets bored' :-)

I'm fine being that "somebody" haha I've had fun writing this plugin.

Do new plugin PRs getting pulled in on the next geany-plugins release?

asifamin13 · 2023-04-25T23:28:11Z

I added support for loading/saving settings via a GKeyFile

asifamin13 · 2023-05-19T16:38:17Z

I've been daily driving this plugin for several months now. I think it's stable enough to be merged.

The latest CI build failure seems to be from misconfigured runners.

elextr · 2023-05-20T01:08:00Z

The CI was awaiting a manual approval to run, approved, lets see.

Ideally needs someone who isn't the author to test it, nothing personal, but we all know how badly we test our own software :-)

asifamin13 · 2023-06-05T20:33:15Z

Hmmm, the other CI builds seem to work without a problem. For some reason this one isn't getting picked up:

This request was automatically failed because there were no enabled runners online to process the request for more than 1 days.

Ideally needs someone who isn't the author to test it, nothing personal, but we all know how badly we test our own software :-)

haha I understand. I'm always open to feedback if yall want to give it a try 😜

eht16 · 2023-06-10T07:55:57Z

Hmmm, the other CI builds seem to work without a problem. For some reason this one isn't getting picked up:
This request was automatically failed because there were no enabled runners online to process the request for more than 1 days.

Maybe this is related to the deprecated Ubuntu 18.04 build configuration in the workflow. Could you try to rebase your branch on current GIT master? The workflow/CI configuration has been changed a lot in the last weeks.
With a bit of luck, the CI will finally work then :).

Add bracketcolors plugin. Color {}, [], () based on nesting order Closes geany#1148

* simple UI * start hooking up signals * mechanism to save settings from a conf file * save load settings for defaults button * less globals * better config color name * hook up colors * glib types * use colors from plugin config * update colors when settings change * reorganze, try to avoid giant files * update document colors when they switch * update headers * fix bug when some brackets werent getting added to bracket map * fix bug that was deleting newly inserted brackets * std::map insert doesnt overwrite, use insert_or_assign * handle when removing a bracket completes another * handle invalid color specs in config file * avoid static initialization fiasco * if there are problems loading config, use defaults * cleanup headers * mark GUI labels as translatable --------- Co-authored-by: Asif Amin <amin@arlut.utexas.edu>

asifamin13 · 2023-07-05T16:53:51Z

@eht16 I did the rebase, verified it compiles/works as expected on my rhel 8 workstation. Shall we give the CI another shot?

eht16 · 2023-07-09T15:35:42Z

@eht16 I did the rebase, verified it compiles/works as expected on my rhel 8 workstation. Shall we give the CI another shot?

The CI is fine.
Though you added quite some new code in the last commit which should be reviewed.

Btw, what is the "#5" referring to you mentioned in the commit message?

asifamin13 · 2023-07-09T16:26:38Z

It's from the pull request on my fork.

I'd appreciate any feedback, I added some GUI elements to specify custom colors on the plugin preferences page.

asifamin13 · 2023-10-09T23:23:42Z

Any chance this could make it into geany-plugins v2.0? 👀

eht16 · 2023-10-15T08:51:55Z

I'm afraid it's too late since the translation string freeze already happened. But we should finally review and merge it after 2.0.

asifamin13 changed the title ~~Bracket colors plugin~~ Add bracket colors plugin Feb 3, 2023

asifamin13 force-pushed the bracketcolors_v1 branch from d005a26 to b75167c Compare February 4, 2023 21:46

asifamin13 mentioned this pull request Feb 4, 2023

Plugin Request: Rainbow parenthesis #1148

Open

eht16 requested changes Feb 5, 2023

View reviewed changes

bracketcolors/README Outdated Show resolved Hide resolved

bracketcolors/src/BracketMap.cc Outdated Show resolved Hide resolved

build/ax_cxx_compile_stdcxx_17.m4 Show resolved Hide resolved

bracketcolors/src/BracketMap.h Show resolved Hide resolved

eht16 reviewed Feb 5, 2023

View reviewed changes

bracketcolors/src/BracketMap.cc Outdated Show resolved Hide resolved

asifamin13 mentioned this pull request Feb 5, 2023

Bracket Colors code review 2/5/23 asifamin13/geany-plugins#2

Merged

asifamin13 force-pushed the bracketcolors_v1 branch from 0ac738a to 0850594 Compare February 5, 2023 21:14

asifamin13 closed this Feb 5, 2023

asifamin13 reopened this Feb 5, 2023

asifamin13 requested a review from eht16 February 5, 2023 22:01

b4n reviewed Mar 9, 2023

View reviewed changes

asifamin13 force-pushed the bracketcolors_v1 branch 2 times, most recently from c1f652e to 1e9a8b0 Compare March 10, 2023 23:58

asifamin13 closed this Mar 11, 2023

asifamin13 deleted the bracketcolors_v1 branch March 11, 2023 00:29

asifamin13 mentioned this pull request Mar 11, 2023

Add bracket colors plugin #1241

Closed

b4n reopened this Mar 15, 2023

b4n force-pushed the bracketcolors_v1 branch from 1e9a8b0 to e8a1d98 Compare March 15, 2023 09:10

asifamin13 force-pushed the bracketcolors_v1 branch from e8a1d98 to 50ae403 Compare March 20, 2023 03:44

asifamin13 force-pushed the bracketcolors_v1 branch from 48fdf9e to 7bef4fd Compare April 6, 2023 17:51

asifamin13 and others added 2 commits July 5, 2023 11:18

Bracket Colors

694d69d

Add bracketcolors plugin. Color {}, [], () based on nesting order Closes geany#1148

asifamin13 force-pushed the bracketcolors_v1 branch from f176f5c to 44de64d Compare July 5, 2023 16:19

Add bracket colors plugin #1221

Are you sure you want to change the base?

Add bracket colors plugin #1221

Conversation

asifamin13 commented Jan 31, 2023

Bracketcolors plugin

How it works

Dependencies

Demo

rdipardo commented Feb 4, 2023

asifamin13 commented Feb 4, 2023

elextr commented Feb 4, 2023

eht16 commented Feb 5, 2023

eht16 left a comment

Choose a reason for hiding this comment

asifamin13 commented Feb 5, 2023

asifamin13 commented Feb 5, 2023

eht16 commented Feb 12, 2023

b4n Mar 9, 2023

Choose a reason for hiding this comment

rdipardo Mar 9, 2023

Choose a reason for hiding this comment

b4n Mar 9, 2023

Choose a reason for hiding this comment

asifamin13 Mar 10, 2023

Choose a reason for hiding this comment

elextr commented Mar 11, 2023

rdipardo commented Mar 12, 2023

elextr commented Mar 12, 2023

b4n commented Mar 12, 2023

b4n commented Mar 12, 2023

elextr commented Mar 13, 2023

asifamin13 commented Mar 14, 2023 • edited Loading

b4n commented Mar 14, 2023

b4n commented Mar 14, 2023

asifamin13 commented Mar 15, 2023

b4n commented Mar 15, 2023

asifamin13 commented Mar 20, 2023

asifamin13 commented Apr 6, 2023

rdipardo commented Apr 7, 2023

elextr commented Apr 7, 2023

asifamin13 commented Apr 18, 2023

asifamin13 commented Apr 25, 2023

asifamin13 commented May 19, 2023

elextr commented May 20, 2023

asifamin13 commented Jun 5, 2023

eht16 commented Jun 10, 2023

asifamin13 commented Jul 5, 2023

eht16 commented Jul 9, 2023

asifamin13 commented Jul 9, 2023

asifamin13 commented Oct 9, 2023

eht16 commented Oct 15, 2023

asifamin13 commented Mar 14, 2023 •

edited

Loading