makew0rld / amfora

A fancy terminal browser for the Gemini protocol.
GNU General Public License v3.0
1.14k stars 65 forks source link

adding tabModeSearch to search in page #240

Open Schubisu opened 3 years ago

Schubisu commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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.

hisacro commented 5 months ago

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

Schubisu commented 5 months ago

@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.