marlon-sousa / EnhancedFindDialog

Enhanced find dialog addon for NVDA, implementing improvements such as search history, optional wraping and others
MIT License
5 stars 6 forks source link

New feature: Allow to find by regexp #24

Open CyrilleB79 opened 11 months ago

CyrilleB79 commented 11 months ago

Hello @marlon-sousa

Sometimes, it would be nice to be allowed to use a regular expression to perform search. Could we add a checkbox for it in the dialog? As for many editors, the checkbox should be disabled by default. This is probably not too hard to implement. I may take care of it, unless you do it before me. Thanks.

Cyrille

marlon-sousa commented 11 months ago

Hello,

PRS are accepted!

Remember to keep the checkbox state persisted by profile, just the way it works with other check boxes.

If you need further assistance let me know.

Thanks!

XLTechie commented 5 months ago

I came here to request this feature, as I needed it today.

marlon-sousa commented 5 months ago

Hello @XLTechie ,

Are you whiling to make a pull request?

If not, I can do it in may be two months.

Here is how I think this should be implemented:

Above the search wrap checckbox, we should have a group or radio buttons called "search mode".

For this first implementation, there should be two radio buttons: "text" and "regexp".

Obviously, selecting "regexp" should apply the content of the search edit field as a regesp, and selecting "text" sghould perform a common text search.

Search history would also need to be respected. For this, we would have to slightly change the way history is stored to include a search mode together with the search term, so that when a previous term is searched we know if it is a text or a regexp search.

The radio button should be adjusted automatically depending of the previous search term type, so that when enter is pressed the correct type of search is performed.

Last but definitely not least ... we would need to save the state of this radio button group in the current active profile, so that it is persisted (the state of the radio buttons, not the search terms) ammong profiles switch, just like the other options are also persisted.

While this is not complex, it would take some time to implement.

Either me or @thgcode will be more than happy to help you with this pr.

XLTechie commented 5 months ago

If I get time, I might take a look at it.

Would you be okay with a checkbox instead of a radio? Along with the wrap checkbox, it makes for only two controls instead of three to tab past, for anyone tabbing.

marlon-sousa commented 4 months ago

Hello,

A radio button fits better because we want to demonstrate that we either want one kind pof search or other.

A regexp search is not a regular search with a plus, which is the case of case sensitivirty, which configures something about a normnal search but still handles a search as a nornal ssearch is.

Same thing for the screen wrapping functionality: it does not change what the search does, it just offers something else on top of that search.

A regexp search is totally different than a normal search. I would arg that it completelly changes the way a search is performed and thus is an exclusive option, you do either a normal or a regexp search.

We can add in the future new modes, for example searchs that will perform only on some specific kind of element, which would allow for extremely rapid and focused searches in web pages.

So I would still stick to radio buttons here.

This, however, is not a hard decision, in the sense that you can implement using a check box (it will be accepted) and when the time comes (other search modes) we can turn it to use radio buttons, however if you could implement like this right now it would be better.

Thanks, Marlon

CyrilleB79 commented 4 months ago

It's worth noting that there is only one tab stop in a radio button grouping, so using a radio button grouping does not add any extra tab in the tab order with respect to a checkbox.

CyrilleB79 commented 4 months ago

I wonder however if this feature would be accepted in NVDA core itself.

marlon-sousa commented 4 months ago

The fact that I have chosen to maintain several addons that should be a part of the core screen reader says much about whether I think this would ever be accepted.

CyrilleB79 commented 4 months ago

@marlon-sousa wrote:

The fact that I have chosen to maintain several addons that should be a part of the core screen reader says much about whether I think this would ever be accepted.

It seems to me that you are a bit discouraged after your attempts to provide NVDA core contributions in the past.

It's true that contributing to NVDA's core is more demanding than creating an add-on; that's all the more true for large contributions implementing a full feature.

A contribution in NVDA's core need to satisfy all users (or at least not create them problems). That's why https://github.com/nvaccess/nvda/pull/8921 was unfortunately reverted: NV Access could not accept to strongly degrade the experience of Asian users using IME. Putting the same feature in an add-on did not cause them issue instead; they just don't have to install the add-on to avoid being impacted. Of course, maybe a solution could have been found to solve the issue with IME and thus keep the feature in core. Unfortunately, you had not the knowledge related to IME to find it, and no one else among contributors or NV Access was able or available to find it.

Re your other contribution attempt https://github.com/nvaccess/nvda/pull/11005: In case you want to turn back to it, I have re-read it and commented there to explain what was still unresolved.

Surely, the find by regexp feature is more tiny, easier to discuss and to implement. If you do not want to discuss it for NVDA core, I may do it later.

marlon-sousa commented 3 months ago

Hello,

This is being worked in pr #30 .

@CyrilleB79 and others please review this draft. Currently, the gui is created. I need specially visually impaired review, as for me the dialog is working as expected, but it might be visually wrong.

Currently only the gui is implemented. Find here the compiled addomn from this pr, the search type will be ignored.

I also inform that #25 will be implemented in this same pr.

XLTechie commented 3 months ago

I apologize--my schedule changed, and I did not have the time to work on this that I intended.

marlon-sousa commented 3 months ago

No need to apologize.If you can review the visual aspects of the dialogue in the draft, it would be extremely helpful.Implementations are on the way.Obrigado,MarlonEm 28 de mai. de 2024, à(s) 07:30, Luke Davis @.***> escreveu: I apologize--my schedule changed, and I did not have the time to work on this that I intended.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

CyrilleB79 commented 3 months ago

@marlon-sousa I have commented in the PR. With a bug fix and other minor fixes, the dialog looks fine!