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

Improve "highlight surrounding quote/bracket" behavior when a selection is active #543

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

tangledhelix
Copy link
Collaborator

Fixes #541

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.

Behavior is a bit odd (though it is exactly as you described and as expected) if you double click a word within quotes to select it, e.g. “double click on this word please”, because the first click causes the quotes to be highlighted then the second removes the highlight. Even if you aren't double clicking, if you just select something to edit it, the quote highlight vanishes then reappears, possibly a bit distractingly, as you work within a quoted or bracketed sentence/paragraph.

Sorry to have only just thought of this, but would a better alternative when there's a selection be to highlight the quotes/brackets that surround the selection (not just the insert cursor)?

I think all that would be needed would be to change highlight_single_pair_bracketing_cursor slightly. At the moment, it gets the insert position into cursor and uses that as the search_from_index argument to the search backward and search forward. Instead, if there's a selection, it could use the start of the first sel range for the search backward loop, and the end of the last sel range for the search forward loop.

Something like this before the first loop:

    if sel_ranges := maintext().selected_ranges():
        cursor = sel_ranges[0].start.index()
    else:
        cursor = maintext().get_insert_index().index()

then this before the second loop:

    if sel_ranges:
        cursor = sel_ranges[-1].end.index()

What do you think?

@tangledhelix
Copy link
Collaborator Author

Sorry to have only just thought of this, but would a better alternative when there's a selection be to highlight the quotes/brackets that surround the selection (not just the insert cursor)?

That seems reasonable, and actually I had wondered briefly about it myself. I didn't do it as I wasn't sure how, but since you suggest what sounds like a workable solution, I'll give that a try.

Instead of only highlighting quotes/brackets around a cursor/insertion
point, also check if a selection is active, and if so, highlight as if
the selection were a cursor.

Fixes DistributedProofreaders#541
@tangledhelix tangledhelix marked this pull request as ready for review November 28, 2024 17:55
@tangledhelix
Copy link
Collaborator Author

Rewrote to look around the selection rather than around the cursor, as @windymilla suggested. And therefore it does highlight even when a selection is active.

@tangledhelix
Copy link
Collaborator Author

Testing notes:

  • highlighting should work as normal when a selection is active
  • just pretend the entire selection is the cursor; what's inside the selection is not looked at or part of the calculation of what to highlight

A few screenshots to illustrate the behavior:

Screenshot 2024-11-28 at 12 58 13 PM

Screenshot 2024-11-28 at 12 58 33 PM

Screenshot 2024-11-28 at 12 58 48 PM


Screenshot 2024-11-28 at 12 59 02 PM

Screenshot 2024-11-28 at 12 59 14 PM

@tangledhelix tangledhelix changed the title Don't highlight surrounding quote/bracket with selected text active Improve "highlight surrounding quote/bracket" behavior when a selection is active Nov 28, 2024
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.

@tangledhelix - I'm not sure if it's an artifact of your rebasing or my fetching of the remote branch, but it appears to me as though it's always using light colors for the cursor line and the align column, even when my theme is dark. Are you seeing the same? If not, I'm happy to go ahead and merge this and untangle anything later once all these related PRs are merged.

@windymilla
Copy link
Collaborator

After a little more investigation, I suspect the tags are being set up before the theme is being applied to maintext

@windymilla
Copy link
Collaborator

windymilla commented Nov 28, 2024

In fact, I don't think there is any way for the code as it stands to change the highlight colors when the theme is changed.
My fault - I said we could create the tags once when maintext was initialized. In fact, it needs to set the tags when the theme is changed including at the point of maintext creation.

Not directly related to this PR - it was the refactoring PR #544, I think, which I obviously didn't test thoroughly enough

@tangledhelix
Copy link
Collaborator Author

In fact, I don't think there is any way for the code as it stands to change the highlight colors when the theme is changed. My fault - I said we could create the tags once when maintext was initialized. In fact, it needs to set the tags when the theme is changed including at the point of maintext creation.

Not directly related to this PR - it was the refactoring PR #544, I think, which I obviously didn't test thoroughly enough

Proposal to resolve that:

  • Store the result of is_dark_theme somewhere, e.g. a class variable on maintext
  • During on_change, check is_dark_theme against that stored value
  • If it's changed, then set the class variable to the new value and reconfigure the tags

@srjfoo
Copy link
Member

srjfoo commented Nov 29, 2024

Working through this one, it looks reasonable (setting aside the theme change issue, which I think y'all have investigated enough that I don't need to at this point 😁). Just a couple of comments that are a result of the way the algorithm works, and I don't know if it's a potential problem or not.

image

You can see that, even though the first of a pair of matching single and doublt straight quotes is highlighted, and disappears, the second of the match in the paragraph does not lose its highlighting because of the matches preceding the paragraph. I know we've mentioned this before, so I'm assuming the answer is still the same. I just wanted to make sure.

@windymilla
Copy link
Collaborator

I know we've mentioned this before, so I'm assuming the answer is still the same. I just wanted to make sure.

Yes, I'm happy with that behavior in this circumstance.

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.

Looks good, thanks

@windymilla windymilla merged commit bd54fa3 into DistributedProofreaders:master Nov 29, 2024
1 check passed
@tangledhelix tangledhelix deleted the issue541 branch November 29, 2024 21:15
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 surround quotes/brackets around selection as if it were a cursor
3 participants