-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Showing all matches during a search #579
Showing all matches during a search #579
Conversation
I've had a quick play - it looks it's on the track I was thinking, and I couldn't break it. I tried to stress it by searching for Searching forward and backward seemed to work, as did wrapping round the end of the file. Of course, colors would need to be discussed. To be honest, I haven't had time to look at the code. Is it adding tags through the whole file (I suspect yes), or is it detecting the start & end visible viewport lines and just highlighting between those? Does it use similar to the highlighting code? |
Yes, and for this PoC I just realized I didn't add a dark mode yet (I think)... I'll try to add those tonight for @srjfoo to review.
At the moment it's just something I rushed out for a PoC, so it's tagging the entire file, I think it even removes and recreates all the tags each time you go to the next occurrence. There are a bunch of things that need refactoring, and others that need to be implemented; e.g. I don't think I check "search in selection" yet. And as you probably saw, it's not very smart about removing the tags; it only does so if you run a new search. |
Pushed some changes:
I was hoping to use the relief border to outline matches. Unfortunately I can't seem to find a way to control the color of the border, it's always black. Therefore in dark mode, it's as if it's not there. (You can tell it is if you look very closely.) I might drop the relief/border for this reason. It's not consistent among the themes. Note that if the text window is not focused, the main match will be different than if it's focused. (As in any case where you are using a selection.) But the inactive matches highlight the same regardless of focus. Please share any feedback about that. This is still buggy, doesn't un-highlight unless you search again, etc... all I did was improve coloring for now. |
I think you're right - if you have relief style "solid", you can't control the color. The other styles choose sensible colors for the theme though, I think. The screenshots below are with style "ridge", which also needs borderwidth at least 2 for it to show the "up" and the "down" of the ridge (discovered through bitter experience during GG1 coding). |
I think this is fine. Since the inactive is a subtle highlight, I think it's OK that it's the same in an unfocused window. |
Bug: if "In Selection" is checked, and a text region is selected before searching, only the active highlight works, and the inactive highlight is not applied. |
a08e998
to
b88c72d
Compare
22e1c3a
to
5cd7fa3
Compare
34e0f00
to
741aa04
Compare
This is not so much a bug in the code as the product of a choice I made to piggyback on |
0eb52b8
to
d1db679
Compare
I decided to do some poking, and thought I'd comment in case it's useful. @windymilla, is "in selection" is supposed to work for a general search. If it is supposed to work, it's broken in This may be limited to Macs, because of the way the search form works: If I search and press the return or the search button, the search form remains active. That means that the actual active selection is pretty difficult to see, because the color of the selection is the background selection color. It's actually not too bad, though, because the line and column numbers are highlighted. I really like the border around the non-active matches, and the fact that they're slightly dimmed. They stand out nicely, whether foreground or background. Just so I could guarantee lots of matches, I chose "to" as one of my search terms. It looks like I did a "find all" on it, because it's got the orange spotlight color. And I can't clear it. Running a subsequent normal search on Unrelated to this PR, but for "find all" + "in selection" -- the only thing that allows one to see the highlighted search terms is if one clears the selection after one does a find all, because the highlighting of the selection overrides the spotlight. |
That's behaving as it should, and I don't have any plans for simple find within selection
It's not limited to Macs. GG1 is the same in that it keeps the focus in the search dialog. The problem for you if it didn't is that IIRC in order to search again, you'd need to click once to get focus back and then again to click the Search button, so 2 clicks for every search. However, GG1 is different in that when search it doesn't highlight the match with a "normal" selection, it uses a thing a bit like our "spotlight". This has the disadvantage IMO in GG1 that if you switch focus to the main window, and type, it doesn't replace the found match (although we could maybe make it do that, I guess, even if we used our own tag, rather than normal selection). Notepad++, for example, does select the found match, as does VS code. If you dismiss the search box, the latest match remains as a normal selection, and typing replaces it. However, we could override the theme and force the inactive selection background color, if we were concerned about it being too subtle.
Yes, they look good, though this (unlikely case - I searched for
That's expected & deliberate behavior for all spotlights. They remain until you do something else that is spotlighting, or you close the dialog that did the spotlighting (Find All dialog in this case). We could possibly argue that Find All is a special case and the spotlight should be cleared when you do another search, but you could also argue for keeping it as it is.
Hmm - not sure what @tangledhelix thinks about that. We definitely want selection as higher priority than spotlight for most cases, as reported by Rick previously when you're trying to fix something that has been spotlighted. Attempting to swap priorities seems dangerous, so does that mean that for Find All only, it needs its own spotlight (which looks the same to the user, but is actually higher priority than selection)? Thinking about swapping priorities, you'd have to have calls to put selection to the top whenever a selection is made, and put spotlight to the top whenever spotlight is shown - seems as though it would be likely to go wrong. |
No to swapping priorities. Maybe when the user navigates to a match from one of these dialogs (Find all, WF, etc,) the navigation could de-select whatever's selected and make the match just jumped to the new selection (or else have no selection at all). |
My observation was just that, an observation, and the only place I observed it was when doing something "in selection" plus another operation. I hope I didn't seem to be suggesting a solution, or even suggesting that there is an easy solution. Perhaps all we should do is document that where there are multiple overlapping highlightings, something has to override. And list the priorities. |
If you select some text and then do a "find all," and click around the matches, if there are matches in your selected region, then you can't see when you jump to them. To me it feels basically like a bug, so IMO it should be addressed. Since I don't think we should change the highlight ranking, and I don't think this behavior is good either, the obvious (to me) solution is just deselect whenever we navigate to a match like that. I haven't checked, but there may even be one method that is used to do such jumps and we could put the code in that one place... |
I guess no selection at all - because if we did a select, then we'd lose the point of having a spotlight style in the first place... |
That sounds like a good plan. Since selection is cleared if the user clicks in the text window, it would make sense if they navigate to the text window via a match from a checker dialog. @tangledhelix, do you want to do that in this PR - I don't think it's related - or shall I do a separate one? |
If the search is "in selection", you still want to see results from within the selected range -- it's not clear to me whether you mean "clear selection" or "clear selection highlighting"? |
It actually does both - clears selection and highlighting. However this is after you have clicked "Find All", so it won't alter which results you see. My suggested solution is equivalent to doing a Find All (which then highlights the first match) clicking in the main window (which will clear the selection) then clicking back on the first match in the Find All dialog. |
I'd rather you do a separate one. I'll probably be tinkering with this one a while yet, and I'm away a bit right now busy with some remodeling work (remodeling house, not code 😄 ) |
fbaba19
to
2f1f16b
Compare
This should be working now, and I'll turn off the Draft mode. It's ready to review. Testing notes:
I believe I changed the colors since earlier reviews - so please mention whether the colors seem proper or not. |
2f1f16b
to
251c82e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks great.
Your <Destroy>
binding works better than the general one in ToplevelDialog
, so I'll steal that idea once this is merged: New issue here
However, one small problem if the main window is destroyed before the Search dialog:
- Open the Search dialog
- Close the main window without first closing the Search dialog
- On Windows, at least, that gives an exception as the Search dialog tries to clear highlighting from maintext which has already been destroyed.
- A suitably placed
if maintext().winfo_exists():
should fix it.
When searching, jump between matches as before when navigating matches, but also highlight all other matches for user convenience. Highlights removed when: - search is changed to a new search string - "Remove Highlights" command is run - Search dialog is closed Fixes DistributedProofreaders#564
251c82e
to
2647ebe
Compare
Same exception on Mac (FWIW). Should be fixed in the latest commit. |
@@ -2902,8 +2928,14 @@ def highlight_selection( | |||
tag_name, match.rowcol.index(), match.rowcol.index() + "+1c" | |||
) | |||
|
|||
def remove_search_highlights(self) -> None: | |||
"""Remove highlights for search""" | |||
if maintext().winfo_exists(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @tangledhelix - I misled you. I think this fix would actually be better if it said
if self.winfo_exists():
If you agree, please change it. If you think it doesn't matter, don't worry. I'm going to approve anyway now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if the following are something to be concerned about, but thought I'd mention them.
-
Selected text shows cursor position, which is not the case for manually selecting text. Mentioning it because it's a visual inconsistency (and because I was the one making such an issue about it not being needed 😁).
-
When switching panes in split screen mode, the "Search" button re-selects the last selected match from before the switch, if switching back and forth between top and bottom. This is true whether using the button or using cmd+g on a Mac -- don't know if it does the same on Windows. Mentioning it because it may or may not be something that will confuse users. It doesn't do it when searching backwards, which gave me the clue as to why it's happening -- it's related to my first point, above. Since the cursor is placed at the beginning of the selected word, it's "found" again when the search is re-done.
Should these be fixed before this gets merged?
(I closed the main window without first closing the search dialog, and confirm that it did not generate an exception.)
Hm ... apologies -- when I did the review earlier, I apparently marked it "request changes". I hope my note was clear enough that I'm not asking for changes if y'all don't agree with me, but I thought I'd go ahead and mention it, anyway. |
I don't think this is related to this PR (i.e. I've observed it in
This is not related to this PR (it's in
I suspect that As for the solution, I'm of the opinion it isn't that important. You can probably see why my algorithm appears to make sense. It's trying to detect that you have done a search, and are now doing another search, and you want the search to find the next one, not find the same one again. GG1 doesn't have the same issue. However GG1 does some stuff where it tries to move on one character before searching, so you can also get weird behavior there. One idea I've just thought of is... Any thoughts welcome. |
Yes, that's exactly it. The found word in the peer pane shows the cursor at the beginning of the found string.
Let me describe what I'm doing:
Example: In my "Land Mammals" test file, I searched for the word This does not happen when searching backwards, presumably because the cursor (whether hidden or not) is at the beginning of the string, and when searching for the next (i.e. previous) occurrence, the same word is not scanned again. Is this any help, or have I just confused the issue more? And no, it doesn't need to be fixed in this PR, yes, it's in |
This is a proof-of-concept for #564.
To try it out, do a search and you should see all matches highlighted in a subtle color, and the currently-active match in a more prominent color. You can go to the next match, etc, and the highlighted match will hop around but the inactive ones will all remain visible.
This is not production quality and has known bugs (for instance, the match highlights are not removed unless you start a new search, because I haven't gone through to find all the places they'd need to be cleaned up yet).
Is this on the track we were thinking? Any notes about the basic functionality before I go tidy up all the cobwebs?
One final note - the current match is also selected, as it has always been, but there's a more prominent highlight applied to the same range, so it's not colored like a selection typically is. But it is selected, and you can check that by typing over it, that should work the same as before.