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

Displays the details of the Match object returned by the Regex call.… #627

Merged
merged 7 commits into from
Sep 18, 2022
Merged

Displays the details of the Match object returned by the Regex call.… #627

merged 7 commits into from
Sep 18, 2022

Conversation

jamescurran
Copy link
Contributor

… Handles much of Issues 76 & 481.

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • UI change (please include screenshot!)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Internationalization and localization
  • Other (please describe):

What is the current behavior?

Regex Tester merely highlights matches.

Issue Number: #76
Issue Number: #481

What is the new behavior?

Additionally, it now also displays a grid at the bottom that shows all the properties of the (.NET CLR) Match objects returned by the Regex call. This is modeled from the display of the Interactive Regex Evaluator example included with LinqPad.

RegExVis

Other information

Two notes:

  • I have not put in that much effort to match the style of the rest of the tools. It does not seem to conflict with them. One area that might violate style standards is where I have a DataGrid inside a DataGrid inside a DataGrid, and used different colored borders to visually sort them out.
  • The headers on the DataGrids have hard-coded English names. I originally tried to localize them, but WPF makes accessing ViewModel.String difficult from three levels deep in DataGrids. But then I realize that these are names of properties of a .NET class, and would need to be referred to by their English names in code, which would be the whole reason for this feature.

Quality check

Before creating this PR, have you:

  • Followed the code style guideline as described in CONTRIBUTING.md
  • Verified that the change work in Release build configuration
  • Checked all unit tests pass

@veler
Copy link
Collaborator

veler commented Aug 27, 2022

Hello,
Sorry for the very late answer to your PR.
I'm very interested by adding this change to DevToys :D
However, as you may noticed, the use of DataGrid isn't ideal because it takes a lot of space, along with wasting a lot of space (on the left in your screenshot).

Here is a suggestion at how we could improve it:

Getting inspired from https://regex101.com/ and using your example of (?<Letters>\w{3})(?<Digits>[1-5]+)-(\w\d\d), what do you think about displaying groups in a flat list like this (but maybe without the colors?)
image

There would still be the group names, the location of the match along with the matched text. But it would overall take much less space and probably be less overwhelming.

Alternatively, or in addition, the matches could be showed as a JSON:

[
  [
    {
      "content": "ABC123-x12",
      "isParticipating": true,
      "groupNum": 0,
      "startPos": 0,
      "endPos": 10
    },
    {
      "content": "ABC",
      "isParticipating": true,
      "groupNum": 1,
      "groupName": "Letters",
      "startPos": 0,
      "endPos": 3
    },
    {
      "content": "123",
      "isParticipating": true,
      "groupNum": 2,
      "groupName": "Digits",
      "startPos": 3,
      "endPos": 6
    },
    {
      "content": "x12",
      "isParticipating": true,
      "groupNum": 3,
      "startPos": 7,
      "endPos": 10
    }
  ],
  [
    {
      "content": "XYZ123-x12",
      "isParticipating": true,
      "groupNum": 0,
      "startPos": 11,
      "endPos": 21
    },
    {
      "content": "XYZ",
      "isParticipating": true,
      "groupNum": 1,
      "groupName": "Letters",
      "startPos": 11,
      "endPos": 14
    },
    {
      "content": "123",
      "isParticipating": true,
      "groupNum": 2,
      "groupName": "Digits",
      "startPos": 14,
      "endPos": 17
    },
    {
      "content": "x12",
      "isParticipating": true,
      "groupNum": 3,
      "startPos": 18,
      "endPos": 21
    }
  ]
]

What do you think?

This was linked to issues Aug 27, 2022
@jamescurran
Copy link
Contributor Author

Yes, all that sounds fine.

Funny thing -- I've already written the JSON output option, along with the Substitution panel (also stolen from regex101.com). They were part of a Phase II update which I was holding off until I got some feedback on this one.

(The UI I have for these is a disaster, but I'll work on it to make it presentable)

@veler
Copy link
Collaborator

veler commented Aug 28, 2022

Yes, all that sounds fine.

Funny thing -- I've already written the JSON output option, along with the Substitution panel (also stolen from regex101.com). They were part of a Phase II update which I was holding off until I got some feedback on this one.

(The UI I have for these is a disaster, but I'll work on it to make it presentable)

Awesome! :D

@jamescurran
Copy link
Contributor Author

Here's a sample of what I have so far. (Still need to put the JSON output in) The first is with the "Replacement pattern" panel closed, and the second, opened and in use.

RegExVis2
RegExVis3-replace

@veler
Copy link
Collaborator

veler commented Aug 29, 2022

It looks much better already! :D Thank you so much!

I really like this list, it's much easier to read.

Also, I think the JSON is something optional. I'd suggest doing either one or the other (the list sounds better to me) and wait to see if customers are actually asking for JSON.

Some feedback on the UI:

  1. Do we really need the grid splitter between Text and Replacement?
  2. There should be some horizontal margin around the Input and Ouput. You can take example from the RegEx options in this UI, or the Settings page.
  3. Is there an actual need for Replacement Pattern? Asking because I can't find any upvoted issue asking for this.

@jamescurran
Copy link
Contributor Author

jamescurran commented Aug 31, 2022

OK, here's the new take. I've taken out the replacement panel and cleaned it up a bit. I've also added a simple error message if there is a problem with the regex pattern:
RegExVis4-Revised

RegExVis5-error

The error message is just the text from the Exception.

I just commented out the replacement panel (I hate deleting code), in case we want to add it back later, but I'm not sure how you feel about cluttering up the source code like that.

@veler
Copy link
Collaborator

veler commented Aug 31, 2022

Hello,
Thank you very much for this great progress! :D

I played a little bit with the UI and came up with the following. What do you think?

image

image

image

It still isn't perfect in my opinion, but it's getting closer from something consistent with how other tools work:

  1. The Text input takes all the space available and gets automatically smaller when the list of matches gets bigger (while having a minimum height).
  2. The error message is now an InfoBar.
  3. The list of matches is now a ListView and the list of matches and groups is now a flat list where I simulated a list of nodes (we could potentially even use an actual TreeView control).
  4. The Range text is stylized appear more grey-ish, like a disabled text.

Remaining details that could potentially be improved:

  1. I wonder if we should hide completely the Matches section when there's nothing to display, or show a text "There is no matches" in the area (or just set 1 item in the ListView that would be something like "No matches")
  2. Should we keep showing groups when the length is 0?
    image

I committed my suggestion in a separated branch here, feel free to take a look: e17c7a3

What do you think?
Thank you for your patience and great work :D

@jamescurran
Copy link
Contributor Author

I like the new look, particularly for the error message. (As I said, my UI skills aren't that good)
I think we should have some form of "no match" message.
I thought I fixed the problem of showing zero-length matches.

@veler
Copy link
Collaborator

veler commented Sep 2, 2022

No worries :) If you want I can add these changes to your branch, or you can do it. Up to you. Please let me know ;-)

Copy link
Collaborator

@veler veler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,
I allowed myself to address the last feedback :)
I will merge this PR, thank you again for this contribution :D

@veler veler linked an issue Sep 18, 2022 that may be closed by this pull request
@veler veler merged commit 20befa6 into DevToys-app:main Sep 18, 2022
@veler veler mentioned this pull request Sep 18, 2022
veler added a commit that referenced this pull request Mar 31, 2023
…#627)

* Displays the details of the Match object  returned by the Regex call.  Handles much of Issues 76 & 481.

* Show Replacement text

* Redesigned details output

* Removed Replacement panel, cleaned up UI.

* Addressed feedback

Co-authored-by: Etienne BAUDOUX <ebaudoux@velersoftware.com>
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.

Extract regex matches Regex highlighter bug Add group support to RegEx Tester tool
2 participants