Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Highlight quote marks & brackets #519

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

tangledhelix
Copy link
Collaborator

@tangledhelix tangledhelix commented Nov 9, 2024

Fixes #42

Adds:

  • bool preference for highlighting quote marks and brackets
  • toggle in the Search menu to turn pref on and off
  • 500ms repeating event to do the highlighting work

Highlights the following pairs:

  • parens ()
  • curly brackets {}
  • square brackets []
  • straight single and double quotes '', ""
  • curly single and double quotes ‘’, “”
  • single and double angle quotation marks (aka guillemets) ‹›, «»
  • single and double "high-reversed-9, low-9" quote marks (‘nines’) ‛‚, ‟„

I created a test file for your convenience (which I'll attach to this PR). In the text file, put your cursor on the all-caps word in the middle of each paragraph for easy highlighting of a bunch of bracketing pairs.

I'm especially interested in feedback on the colors, including in the different themes. The colors seem fine to me in default & light theme on Mac. I'm less sure they're quite right in dark mode; and I haven't tested on other than Mac.

quotes-test.txt

@tangledhelix
Copy link
Collaborator Author

I forgot to mention - a decent amount of the content here is porting some of the stuff from Perl's Tk module. I used this as the source material:

https://metacpan.org/release/SREZIC/Tk-804.036/source/Tk/TextEdit.pm

Copy link
Collaborator

@windymilla windymilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code and behavior all look good to me, apart from when using View-->Split text window.

I think it's to do with when you get the min & max line number that are visible, and use that to determine where to do the highlighting. The problem is that with the split view the min & max visible line numbers in the maintext window may not be visible in the "peer" (the bottom half of the split) so there will be no highlighting if you scroll down and click in the bottom half of the screen. I think the fix is to use
(top_frac, bot_frac) = maintext().focus_widget().yview()
calling focus_widget so that it gets the top/bot fractions based on the widget that has been clicked in whether it's the main text or the peer.

@tangledhelix
Copy link
Collaborator Author

I think the fix is to use (top_frac, bot_frac) = maintext().focus_widget().yview()

Based on my testing, looks like you're correct. Included that.

Copy link
Collaborator

@windymilla windymilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks

@tangledhelix
Copy link
Collaborator Author

I added #521 to separately review the colors.

@srjfoo
Copy link
Member

srjfoo commented Nov 10, 2024

I added #521 to separately review the colors.

Thanks. I got started on the review yesterday, and started down the color rabbit hole and, of course, got stalled. This allows me to actually focus on the rest of the PR. :D


I haven't finished the review yet, but in case this is something that needs to be fixed, I thought I'd go ahead and mention it. Is there any reason that there isn't a checkmark beside "Highlight Double Quotes in Selection" (same for single):

image

@tangledhelix
Copy link
Collaborator Author

I haven't finished the review yet, but in case this is something that needs to be fixed, I thought I'd go ahead and mention it. Is there any reason that there isn't a checkmark beside "Highlight Double Quotes in Selection" (same for single):

That is mirroring what GG1 does.

"Highlight Double Quotes in Selection" (and single) are one-time commands, they highlight one time based on the currently selected text.

"Highlight Quotes & Brackets" (also "Highlight Alignment Column") are modes you turn on or off, and they continuously update as the text is changed and as the cursor position moves.

@srjfoo
Copy link
Member

srjfoo commented Nov 10, 2024

I'm especially interested in feedback on the colors, including in the different themes. The colors seem fine to me in default & light theme on Mac. I'm less sure they're quite right in dark mode; and I haven't tested on other than Mac.

"Highlight Single/Double Quotes in Selection" does not show a checkmark when it's active -- at least not for me. There should be some indication in the menu that it's active, I think?

The checkmark does show for "Highlight Surrounding Quotes & Brackets". But that can be involed only at the paragraph level.

The differences betwee the two threw me for a bit. The "in selection" is obvious in retrospect. The selection can be as large or as small as needed, and there's only one highlight color. I think that's the system one? At any rate, it's very different from "Highlight Surrounding".

So, on to "HIghlight Surrounding". Initially, I thought the variety in colors was associated with nesting level, but it's not -- each color is associated with a particular type of markup. Is that easier than having a rotating set of, say, three nesting level colors? That way you don't have to come up with a large number of different colors.

I've looked at three options:

  1. light OS in macOS, default theme
  2. dark OS in macOS, default theme
  3. dark OS in macOS, dark theme.

Light OS is just too bright for me most of the time. I'll leave it to someone who uses it to determine if the colors work.

Dark OS, default theme is my preferred setting. And checking the actual dark theme, I didn't see any difference worth mentioning.

What works best for me is the darker colors with white text. There is one choice that I've seen that has black text (the single angle brackets), and I don't really like it. (Also, consider that »« and ›‹ are legitimate forms of the angle brackets.

So, my question here is, did you implement it this way because it's easier to have one set of colors for each type of punctuation than it would be to have a rotating set of, say, three colors that are linked to nesting level? How often will more than nesting levels be needed?

<tb>

I haven't finished the review yet, but in case this is something that needs to be fixed, I thought I'd go ahead and mention it. Is there any reason that there isn't a checkmark beside "Highlight Double Quotes in Selection" (same for single):

That is mirroring what GG1 does.

"Highlight Double Quotes in Selection" (and single) are one-time commands, they highlight one time based on the currently selected text.

"Highlight Quotes & Brackets" (also "Highlight Alignment Column") are modes you turn on or off, and they continuously update as the text is changed and as the cursor position moves.

And yet the highlighting stays turned on for "Highlight Double Quotes in Selection" until I explicitly remove it. Regardless of whether or not it's mimicking GG1, should it stay turned on until removed? (Or am I missing something obvious in how to get rid of the highlighting?

@windymilla
Copy link
Collaborator

The checkmark does show for "Highlight Surrounding Quotes & Brackets". But that can be invoked only at the paragraph level.

Not sure what you mean by "invoked only at the paragraph level", but if you mean it only works within a paragraph, that's not true. It will find an open bracket in one paragraph that is matched by a close in a later paragraph. It searches backwards from the cursor to find the closest open bracket, for example, then forwards to find the closest close bracket, and then highlights them. That explains the name "Surrounding" - it means "surrounding the insert cursor".

It's an old GG1 feature, that used to behave (as Dan described) in a useful way for a code editor, e.g. matches had to be on on the same line or aligned, and it didn't "work" unless you modified the text between the matching pairs. I continued to use the same built-in feature but put a different wrapper round it, similar to what Dan has done (though he had to write the built-in bit too!). The GG1 change was here: DistributedProofreaders/guiguts#789
Looks as though that was before you were roped in to do GG1 testing!

@srjfoo
Copy link
Member

srjfoo commented Nov 10, 2024

The checkmark does show for "Highlight Surrounding Quotes & Brackets". But that can be invoked only at the paragraph level.

Not sure what you mean by "invoked only at the paragraph level", but if you mean it only works within a paragraph, that's not true. It will find an open bracket in one paragraph that is matched by a close in a later paragraph. It searches backwards from the cursor to find the closest open bracket, for example, then forwards to find the closest close bracket, and then highlights them. That explains the name "Surrounding" - it means "surrounding the insert cursor".

It's an old GG1 feature, that used to behave (as Dan described) in a useful way for a code editor, e.g. matches had to be on on the same line or aligned, and it didn't "work" unless you modified the text between the matching pairs. I continued to use the same built-in feature but put a different wrapper round it, similar to what Dan has done (though he had to write the built-in bit too!). The GG1 change was here: DistributedProofreaders/guiguts#789 Looks as though that was before you were roped in to do GG1 testing!

My apologies -- the bit you quoted was from a first draft of a comment that I forgot to delete and apparently got posted -- I can go back and edit out (or delete the whole comment, but most if it is irrelevant to things I learned later.

@windymilla
Copy link
Collaborator

No problem

@tangledhelix
Copy link
Collaborator Author

The differences betwee the two threw me for a bit. The "in selection" is obvious in retrospect. The selection can be as large or as small as needed, and there's only one highlight color. I think that's the system one? At any rate, it's very different from "Highlight Surrounding".

I don't think it's a system-defined color; it's a color taken from the same feature in GG1, I believe (highlight.py: HighlightColors.QUOTEMARK).

So, on to "HIghlight Surrounding". Initially, I thought the variety in colors was associated with nesting level, but it's not -- each color is associated with a particular type of markup. Is that easier than having a rotating set of, say, three nesting level colors? That way you don't have to come up with a large number of different colors.

You're correct, it doesn't relate to nesting, it just relates to the type of character. Each one is different intentionally here. In GG1 I believe it's the same. My presumption is that they are colored differently so you can easily spot which starting character matches to which ending character, when more than one type is present.

What works best for me is the darker colors with white text. There is one choice that I've seen that has black text (the single angle brackets), and I don't really like it. (Also, consider that »« and ›‹ are legitimate forms of the angle brackets.

Hmm. The way this algorithm works that would be very hard to differentiate from the other direction.

Guillemets and the "high/low nine" forms weren't implemented in GG1. I debated including the nines forms at all since they're fairly uncommon. And I wasn't aware of guillemets being reversible like that. I'm not sure how to handle that without having some sort of configurable item letting the PP'er choose which way they're facing in a given project. Or is it by language? Maybe the direction could be determined dynamically by the project language.

googles...

I see there's a Wikipedia page on this. And that Finnish and Swedish even use »this form»! So maybe I can just encode how it works by what language the document is marked as?

And yet the highlighting stays turned on for "Highlight Double Quotes in Selection" until I explicitly remove it. Regardless of whether or not it's mimicking GG1, should it stay turned on until removed? (Or am I missing something obvious in how to get rid of the highlighting?

No, you're not missing anything. It's designed to stay on until removed with the "Remove Highlights" command. And it is mimicking GG1 behavior here.

@srjfoo
Copy link
Member

srjfoo commented Nov 11, 2024

And another apology for accidentally posting the original you quoted without heavy editing.

The differences betwee the two threw me for a bit. The "in selection" is obvious in retrospect. The selection can be as large or as small as needed, and there's only one highlight color. I think that's the system one? At any rate, it's very different from "Highlight Surrounding".

I don't think it's a system-defined color; it's a color taken from the same feature in GG1, I believe (highlight.py: HighlightColors.QUOTEMARK).

👍

So, on to "HIghlight Surrounding". Initially, I thought the variety in colors was associated with nesting level, but it's not -- each color is associated with a particular type of markup. Is that easier than having a rotating set of, say, three nesting level colors? That way you don't have to come up with a large number of different colors.

You're correct, it doesn't relate to nesting, it just relates to the type of character. Each one is different intentionally here. In GG1 I believe it's the same. My presumption is that they are colored differently so you can easily spot which starting character matches to which ending character, when more than one type is present.

Yeah, I figured that out, too -- but I wrote these notes up originally before bringing up GG1!

What works best for me is the darker colors with white text. There is one choice that I've seen that has black text (the single angle brackets), and I don't really like it. (Also, consider that »« and ›‹ are legitimate forms of the angle brackets.

Hmm. The way this algorithm works that would be very hard to differentiate from the other direction.

Guillemets and the "high/low nine" forms weren't implemented in GG1. I debated including the nines forms at all since they're fairly uncommon. And I wasn't aware of guillemets being reversible like that. I'm not sure how to handle that without having some sort of configurable item letting the PP'er choose which way they're facing in a given project. Or is it by language? Maybe the direction could be determined dynamically by the project language.

googles...

I see there's a Wikipedia page on this. And that Finnish and Swedish even use »this form»! So maybe I can just encode how it works by what language the document is marked as?

Does it make sense to do that, or does it make more sense to strip down to the most common basics and have a user defined single and a user defined double quote pair? I won't be offended if that sounds like a bad idea to y'all -- you're doing the coding. 😁

As for the unidirectional guillemets, they're functionally the same as straight quotes, aren't they?

The bits that I read, it sounded like German didn't know exactly what they wanted to do. Sometimes they used the French orientation, sometimes high-low, and sometimes they reversed the French orientation.

And yet the highlighting stays turned on for "Highlight Double Quotes in Selection" until I explicitly remove it. Regardless of whether or not it's mimicking GG1, should it stay turned on until removed? (Or am I missing something obvious in how to get rid of the highlighting?

No, you're not missing anything. It's designed to stay on until removed with the "Remove Highlights" command. And it is mimicking GG1 behavior here.

That's what I was missing. <sigh> ... I guess it would only make sense for there to be a checkmark by the menu choice if it was turned on for the whole document, so "Remove Highlights" makes more sense; I just didn't spot it. Can we move "Remove Highlights" above "Highlight Surrounding Quotes & Brackets", since it doesn't apply to that, just to the "Highlight ... Quotes in Selection"?

@tangledhelix
Copy link
Collaborator Author

Removed the 500ms timer cadence and now calling from on_change instead.

@windymilla
Copy link
Collaborator

Removed the 500ms timer cadence and now calling from on_change instead.

That appears to work correctly, except when you first turn it on:

  1. Turn off "Highlight surrounding..."
  2. Place cursor between 2 quotes or brackets
  3. Turn on "Highlight surrounding..."
  4. No highlighting appears until you click, type, use the arrow keys or scroll

Turning it off does seem to hide the highlighting immediately.

Probably need to revert the edit to highlight_quotbrac_callback so it still calls the highlighting routine when value is True.

@tangledhelix
Copy link
Collaborator Author

That appears to work correctly, except when you first turn it on:

Ah, good catch, I missed that entirely! Fixed by calling immediately in the callback.

When activated from the Search menu, this mode finds pairs of quote
marks and brackets surrounding the cursor position and highlights them.
Only pairs are highlighted, and only the nearest surrounding pair (of
each type) surrounding the cursor is highlighted.

Also fixes a failing test (on macOS) in pytest.

Fixes DistributedProofreaders#42
@tangledhelix
Copy link
Collaborator Author

Squashed and rebased.

@srjfoo
Copy link
Member

srjfoo commented Nov 15, 2024

Color suggestions, bearing in mind that I'm only testing on default with macOS on the dark theme.

  • For curly double quotes, change limegreen to darkgreen (white may show up well in the samples at w3schools, but with smaller text, it doesn't do too well). If you don't think the contrast is enough between the curly and straight double quotes, switch one of them to a different color, like firebrick, darkred, brown or chocolate
  • Change straight single quote to dimgrey (which is actually darker than grey or darkgrey). It needs just a little bit more contrast.
  • For the parens, how about fuchsia? The existing color is too pale for the white text.
  • For QUOTEMARK (highlight in selection), how about going a bit paler, say c1b4fd. The black shows up a bit better to me.

And speaking of the QUOTEMARK color, it occurred to me as I was testing that that it kind of makes sense for the "highlight in selection ..." and the "highlight surrounding ..." to have something to differentiate them. You've got all the "highlight surrounding" using white text on a background that's hopefully dark enough to have decent contrast, but for "highlight in selection", two have black text and one has white text, with the same background colors. Why not just make all of them have black text (maybe bolded?) on a paler background?

I found this useful in testing, if I happened to have "highlight surrounding" turned on, and then did a "highlight in selection", and I can imagine the same might happen to others.

I did a screenshot of both versions -- what's in the PR, and with my suggested color changes, and applied a filter in photoshop that displayed them in greyscale. Both have good points. And neither completely differentiates the contrast for all pairs. My suggestion is settle on colors that work, and if someone comes along who has trouible with those colors recruit them to help find better choices. (Also remembering there are different kinds of color blindness.)

I'm okay with it the way it is, but I would like to see the color changes.

@windymilla windymilla merged commit af20679 into DistributedProofreaders:master Nov 15, 2024
1 check passed
@windymilla
Copy link
Collaborator

Colors will be addressed in #521

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Highlight surrounding quotes/brackets
3 participants