adding tabModeSearch to search in page #240

Schubisu · 2021-05-30T18:01:18Z

Addressing #36 I tried to follow up on that matter, because I really would love to have the 'search in page' feature implemented. I couldn't find the code that @lachrimae already worked on and I never really wrote a line of go before, so I expect that some of my edits can easily be improved. I'm thankful for any suggestions to make this PR better.

Instead of adding a "search" region tag, I add numbered tags to the matching strings, to make use of cview's ScrollToHighlight function. I added a new tabMode to block other keypresses like follow links, as suggested in #36.

The way this is implemented, a search can potentially mess up other regions (e.g. if searched for special characters like # or >). I'm currently not sure how to address this best. I have not used regular expressions, not because I don't know how, but because I don't know how they can solve this problem. However, with an Esc keypress, the original buffer will be restored.

makew0rld · 2021-06-24T19:07:06Z

Hello, thanks for your contribution and interest in Amfora!

Instead of adding a "search" region tag, I add numbered tags to the matching strings, to make use of cview's ScrollToHighlight function.

ScrollToHighlight scrolls so the highlighted regions are visible. But any region IDs can be made visible with Highlight, they don't need to be numbered strings.

The way this is implemented, a search can potentially mess up other regions (e.g. if searched for special characters like # or >). I'm currently not sure how to address this best. I have not used regular expressions, not because I don't know how, but because I don't know how they can solve this problem.

Unfortunately it's pretty important to me that no page content gets messed up during searching. Regular expressions are needed because the content of the rendered version of the page can contain cview color tags like [red] or [red:white:]. These tags are invisible, but change things like text color and style. If you searched for a string like "red" it would come up in these tags, but obviously the user does not want that result. And if you try to inject any region tags, it would mess up that tag entirely.

This is why searching for "#" causes issues, because a lot of the tags in your page look like [#a3f4bb] or something, and then trying to insert a numbered tag inside it breaks the color.

The solution to this is to use regexes to detect what's a tag and what's not, and skip over highlighting the things that are. This is all explained in #36. The steps needed to be implemented and the regex to use are all there.

If you'd like to update this PR to do that, I can leave it open, otherwise I'll close it. I appreciate you taking a stab at this, unfortunately it's trickier than any of us would like it to be.

Schubisu · 2021-06-25T07:50:44Z

Hi @makeworld-the-better-one, thanks for your reply and suggestions.
Yes, I already thought that this would be the reason for not having a search implemented yet. Personally, to me a search function is more important than to leave the tags intact, given that the original buffer can be reloaded anytime with an Esc keypress. But I would like to try and make this PR better and implement the regex.

ScrollToHighlight scrolls so the highlighted regions are visible. But any region IDs can be made visible with Highlight, they don't need to be numbered strings.

It's already some time ago that I looked into that, but I think that ScrollToHighlight does not work well if all region IDs are the same, because it will always scroll to the first region. That's why I numbered the regions to scroll to the next occurrence on tab press.

Please leave this PR open, so I can continue to work on this, and maybe others could also jump in. I think that I would have profited from the previous efforts that have been made in this direction, if the code had been visible here.

makew0rld · 2021-06-25T17:29:44Z

But I would like to try and make this PR better and implement the regex.

Good to hear, thanks!

It's already some time ago that I looked into that, but I think that ScrollToHighlight does not work well if all region IDs are the same, because it will always scroll to the first region. That's why I numbered the regions to scroll to the next occurrence on tab press.

Ah, you're right. Adding numbered region IDs will conflict with the link region IDs though, which are just numbers right now. Maybe for this the IDs could be like search-0, search-1, etc. That way it's still iterable.

Schubisu · 2021-06-26T09:50:37Z

@makeworld-the-better-one thanks again ;) I tried to implement all your suggestions; it seems to work well. The only flaw I currently see is that no search results are found inside of brackets that are no tags. Maybe that's an easy fix in the tagsRegex but I didn't find it, yet. Is it possible to switch between rendered and raw view inside amfora? That would be quite helpful.

makew0rld · 2021-06-26T19:50:13Z

Thanks for updating. I will review this PR in full soon. For now I came up with a new tags regex to try:

\[[a-zA-Z0-9_,;: \-\."#]+[^\[]*\]

Let me know how that works, and if it solves the issue or creates new ones.

Is it possible to switch between rendered and raw view inside amfora? That would be quite helpful.

The raw view is stored inside each page struct. Not sure if you meant displaying the raw text, which would require escaping it then displaying it. There's no simple way to switch to displaying it built-in, but if you want to look at the raw page contents from the network, that's available in the struct.

Also there are some linter errors to look at.

makew0rld

Thanks again for adding this! Super cool to see it in action. Please take a look at my comments above and below.

Scroll position isn't saved after exiting search, so if I go back and go forward again it won't be in the right place
Making two full copies of the page content feels very inefficient. I can't really think of a better way though, let me know if you have any ideas.
Please replace ScrollToHighlight with t.scrollToHighlight. This was just added to master so you'll have to update your branch here first.

makew0rld · 2021-06-27T17:10:40Z

config/config.go

+	viper.SetDefault("keybindings.bind_search", "/")
+	viper.SetDefault("keybindings.bind_next_match", "n")
+	viper.SetDefault("keybindings.bind_prev_match", "p")


These keybindings need to be added to default-config.toml and to help.go. Don't forget to run make gen after doing that as well.

Also please change these keybindings to match Vim/less. See this comment.

makew0rld · 2021-06-27T17:12:21Z

display/display.go

@@ -28,6 +28,12 @@ var termH int
 // The user input and URL display bar at the bottom
 var bottomBar = cview.NewInputField()

+var originalText []byte
+var searchBar = cview.NewInputField()


Why a separate bar instead of using the existing one? Also this primitive would need to be themed like the bottom bar is. But I think just keeping one bar is better.

makew0rld · 2021-06-27T17:17:31Z

display/display.go

+			searchIdx := searchRegex.FindAllIndex(originalText, -1)
+
+			// find all positions of tags
+			tagsRegex := regexp.MustCompile(`\[.*?[^\[]\]`)


This regex should be changed as I noted in my previous comment, and also it can be compiled once globally, instead of in this function.

makew0rld · 2021-06-27T17:22:56Z

display/display.go

+			// with the actual search strings replaced by tagged regions
+			// to highlight.
+			for i, match := range searchIdx {
+				isMatch = true


Isn't this variable unnecessary? You already break out of the loop if it's false, so the check on 313 isn't needed either.

makew0rld · 2021-06-27T17:23:34Z

display/display.go

+			tagsRegex := regexp.MustCompile(`\[.*?[^\[]\]`)
+			tagsIdx := tagsRegex.FindAllIndex(originalText, -1)
+
+			text := []byte("")


Suggested change

text := []byte("")

text := make([]byte, 0)

makew0rld · 2021-06-27T17:25:02Z

display/display.go

+				if isMatch {
+					matches++
+					text = append(text, originalText[lastMatch:match[0]]...)
+					replacement := []byte(fmt.Sprint("[\"search-", i, "\"]", searchString, "[\"\"]"))


Suggested change

replacement := []byte(fmt.Sprint("[\"search-", i, "\"]", searchString, "[\"\"]"))

replacement := []byte(fmt.Sprint(`["search-`, i, `"]`, searchString, `[""]`))

Using raw strings makes this much more readable.

makew0rld · 2021-06-27T17:30:01Z

display/display.go

+			case config.CmdInvalid:
+				if event.Key() == tcell.KeyEsc {


You can ignore this case and just check the key with an if statement outside the switch.

hisacro · 2024-03-28T07:49:05Z

can we have this as experimental feature? would be helpful even it is bugs

Schubisu · 2024-03-28T13:49:58Z

@hisacro thanks, I completely forgot about this PR, but it's nice to you're interested ;)

@makew0rld my apologies for not following up on this earlier, and thank you so much for your thorough review. I think I have addressed all the points you mentioned, and tried to make small and clear commits for most of them. Please let me know if anything else is needed.

Schubisu

I think this should be fine this way, not sure about official review workflow in github. I guess I hit the "Submit review" button now. Please let me know if I can make any further improvements!

Schubisu · 2024-03-28T13:54:09Z

display/thanks.go

@@ -33,4 +33,5 @@ Thank you to the following contributors, who have helped make Amfora great. FOSS
 * Maxime Bouillot (@Arkaeriit)
 * Emily (@emily-is-my-username)
 * Autumn! (@autumnull)
+* William Rehwinkel (@FiskFan1999)


I'm actually not sure why this addition is listed here, it happened during merge of the master branch, I think

makew0rld · 2024-10-09T17:32:37Z

Thanks. The diff on this doesn't look too bad, but in the near term I don't have time to test and add new features to Amfora. If you keep this PR open I might get to it eventually. You can see the readme for details.

adding tabModeSearch to search in page

36f765e

Schubisu added 3 commits June 26, 2021 11:15

skipping tag regions in text search

441b608

formatted and fixed linter errors

2c5d984

fixed non-exhaustive switch case

8c69506

makew0rld mentioned this pull request Jun 27, 2021

Remove left margin when scrolling #197

Closed

makew0rld requested changes Jun 27, 2021

View reviewed changes

Robin Schubert added 7 commits March 28, 2024 12:50

merge master

b6da629

adding keybindings to default-config.toml and help.go

6c8fde7

change search keybindings to vim/less defaults

84767a3

updated tag regex and compiled it once globally

eaf4460

use existing bottomBar for search

1a59487

adding resetSearch helper function

88caea0

remove redundant bool

5754768

Schubisu commented Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding tabModeSearch to search in page #240

adding tabModeSearch to search in page #240

Schubisu commented May 30, 2021

makew0rld commented Jun 24, 2021

Schubisu commented Jun 25, 2021

makew0rld commented Jun 25, 2021

Schubisu commented Jun 26, 2021

makew0rld commented Jun 26, 2021

makew0rld left a comment •

edited

Loading

makew0rld Jun 27, 2021

makew0rld Jun 27, 2021

makew0rld Jun 27, 2021

makew0rld Jun 27, 2021

makew0rld Jun 27, 2021

makew0rld Jun 27, 2021

makew0rld Jun 27, 2021

hisacro commented Mar 28, 2024

Schubisu commented Mar 28, 2024

Schubisu left a comment

Schubisu Mar 28, 2024

makew0rld commented Oct 9, 2024

	replacement := []byte(fmt.Sprint("[\"search-", i, "\"]", searchString, "[\"\"]"))
	replacement := []byte(fmt.Sprint(`["search-`, i, `"]`, searchString, `[""]`))

adding tabModeSearch to search in page #240

Are you sure you want to change the base?

adding tabModeSearch to search in page #240

Conversation

Schubisu commented May 30, 2021

makew0rld commented Jun 24, 2021

Schubisu commented Jun 25, 2021

makew0rld commented Jun 25, 2021

Schubisu commented Jun 26, 2021

makew0rld commented Jun 26, 2021

makew0rld left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hisacro commented Mar 28, 2024

Schubisu commented Mar 28, 2024

Schubisu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makew0rld commented Oct 9, 2024

makew0rld left a comment •

edited

Loading