matoom / frostbite

MUD client for dragonrealms.
29 stars 17 forks source link

Text highlight feature also replaces text #55

Closed umbravi closed 5 years ago

umbravi commented 5 years ago

When adding a text highlight of "% (enthralled|nearly locked|mind lock)" the unmatched characters, "%" symbol and "\s", are cut from the output. The expectation is that the regex groups identified have the selected color applied, and all characters remain with the same format.

Note: The current function of the highlight feature is acting as a substitute and a highlight, when the expectation, and I believe the intention, is to only highlight text.

Highlight text: % (enthralled|nearly locked|mind lock) color selected: #aa0000

Expectation: Tactics: 66 62% <#aa0000>enthralled<#aa0000>

Actual: Tactics: 66 62<#aa0000>enthralled<#aa0000>

matoom commented 5 years ago

You're right. I think i originally intended to build it around group capturing to allow a little more intricate matching but it's completely unintuitive to use and it doesn't even work properly. What's happening here is that it's only picking up the part that's enclosed in parentheses but adding more groups doesn't work either.

It should be much better if entire matches are captured as a whole and are not split up into groups.

I want to test a few things out first but i'll make a new release soon.

matoom commented 5 years ago

I have the windows version ready, letting it settle a little bit for mac and linux.

https://github.com/matoom/frostbite/releases/tag/v1.9.5-beta

umbravi commented 5 years ago

The highlight function is now not replacing the text, but one issue remains where the color highlighting is applied to the entire string and not only the match groups in the regex. Is there a way for you to implement it so only the groups get the color highlight?

Using the same example from above and a new example for contrast: Highlight text: % (enthralled|nearly locked|mind lock) Color selected: #aa0000

Expectation: Tactics: 66 62% <#aa0000>enthralled</#aa0000>

Actual: Tactics: 66 62<#aa0000>% enthralled</#aa0000>

-and-

Highlight text: The (work) proceeds as planned (without any mistakes) whatsoever. Color Selected: #43ebc1

Expectation: The <#43ebc1>work</#43ebc1> proceeds as planned <#43ebc1>without any mistakes</#43ebc1> whatsoever.

Actual: <#43ebc1>The work proceeds as planned without any mistakes whatsoever.</#43ebc1>

matoom commented 5 years ago

Yea, that's exactly how it was intended to work but it didn't. For a reason. The qt regexp api doesn't really do any replacing, it just finds matches.

I'm currently using the older api though, i'll have to take a look at the new one and see if it opens up any new ways to approach this.

umbravi commented 5 years ago

I can see in the file where you were going for that in the last version. I'm not sure how you feel about this, but you could do a string replace to insert the color tags at the non-escaped parentheses. It may feel a bit hacky, but I suspect it may be a lot more efficient than looping through the string multiple times. I'm not familiar with qt or c++ though.

matoom commented 5 years ago

I'm not sure if i understand. A positive match actually gives pretty limited options to work with. I only have access to captured groups as plain strings, with no way of knowing the exact location of the parentheses.

There's no literal representation of the parentheses in the target text. I can't do a replace("(", "<color>") or replace(")", "</color>").

I could replace the captured entities individually but that would kind of defeat the purpose of regex matching entirely.

The actual replace function can do something similar but it's not really feasible in a non programmatic way.

For example:

replace(QRegularExpression("% (enthralled|nearly locked|mind lock)"), "% <color>\\1</color>");
replace(QRegularExpression("Lorem ipsum dolor sit (amet), consectetur adipiscing elit, sed do (eiusmod) tempor incididunt ut labore et dolore magna aliqua."), "Lorem ipsum dolor sit <color>\\1</color>, consectetur adipiscing elit, sed do <color>\\2</color> tempor incididunt ut labore et dolore magna aliqua.");

This is actually something you can do with text substitutions:

"% (enthralled|nearly locked|mind lock)" -> "% <span style="color:blue">\1</span>"
umbravi commented 5 years ago

That makes sense. Forgive my c++ ignorance.

matoom commented 5 years ago

A quick update. It turns out i was wrong. It seems the regexp api actually does provide group matching locations and it's a pretty small change but i need to figure out how to approach this exactly. I'm kind of leaning towards expanding the highlight ui to include another option to allow matching entire phrases or make it group based. I'm not sure if it's still intuitive to use or make it even more convoluted.

I don't have any ETA yet but something is probably coming at some point.

umbravi commented 5 years ago

Thanks for the update. I've paused setting up any highlights. Without group based highlighting the colors will appear on things I don't want highlighted, experience levels is one case, but there are a few.

I'm glad this is in the pipeline and I'm eager to see it released.

Again, thanks for your efforts!

On Tue, Sep 17, 2019 at 12:50 PM matoom notifications@github.com wrote:

A quick update. It turns out i was wrong. It seems the regexp api actually does provide group matching locations and it's a pretty small change but i need to figure out how to approach this exactly. I'm kind of leaning towards expanding the highlight ui to include another option to allow matching entire phrases or make it group based. I'm not sure if it's still intuitive to use or make it even more convoluted.

I don't have any ETA yet but something is probably coming at some point.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/matoom/frostbite/issues/55?email_source=notifications&email_token=ADQDAJP5ZAS47LRCAHGNMATQKEYIJA5CNFSM4IVQWIU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD65WI6Q#issuecomment-532374650, or mute the thread https://github.com/notifications/unsubscribe-auth/ADQDAJMZLP2D3DZQJWNTTALQKEYIJANCNFSM4IVQWIUQ .

matoom commented 5 years ago

I moved things around a little bit and added a couple of new toggles to the highlighting window. It should now be possible to toggle highlight by regex groups and if not toggled it will default to highlighting the entire match phrase. This way all legacy settings will remain intact and all new functionality is treated as extra settings. I completely removed the enable regex option because that was really never a thing (it's all regex underneath). If partial matching isn't enable it will look for whole words enclosed in word boundaries (\b). You basically want to enable partial matching and regex groups matching to do what you want. The problem is that usability wise i don't know if it's again very intuitive to use but i don't have any ideas on how to improve it either as breaking legacy settings is a concern as well. Try it out and if you have any feedback let me know.

https://github.com/matoom/frostbite/releases/download/v1.9.5-beta/frostbite-win.zip

umbravi commented 5 years ago

Verified my cases work as expected. I'll close this issue, and if anything new pops up I'll reference this issue in the new issue.

Thanks for your efforts!