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

Added Search for Hex functionality to the unified SearchReplace dialog. #608

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

MKadaner
Copy link
Contributor

Summary

Added Search for Hex functionality to the unified SearchReplace dialog.

References

Checklist

Details

@MKadaner MKadaner force-pushed the mzk-stddlg-search branch 2 times, most recently from 385bbd9 to 8680b56 Compare January 17, 2023 03:32
@MKadaner
Copy link
Contributor Author

@rohitab, Could you please test this draft PR? There are five things to test:

  • The behavior of Editor Search / Replace dialog. For testing purposes, I enabled Hex edit controls. It's UI/UX only; there is no attempt to actually implement Hex search in Editor.
  • The behavior of Help Search dialog.
  • The search functionality in Help. The changes in this area were trivial, so it does not require much attention.

Comment out Hex controls in Editor Search / Replace dialog here: https://github.com/MKadaner/FarManager/blob/8680b5639d6b3359674a156d0fbc86a84429e151/far/editor.cpp#L156

Rebuild and test:

  • The behavior of Editor Search / Replace dialog without Hex controls; this is how it should be in production.
  • The search functionality in Editor. I did some refactoring here; could introduce bugs.

Thank you in advance!

@MKadaner
Copy link
Contributor Author

@alabuzhev, Could you please look at this change? It is practically ready except for the testing.

@alabuzhev
Copy link
Contributor

alabuzhev commented Jan 17, 2023

@alabuzhev, Could you please look at this change? It is practically ready except for the testing.

Of course. It is rather massive so probably will take a few days.

Does it actually work though?
Ah, I see, it doesn't:

there is no attempt to actually implement Hex search in Editor.

far/editor.cpp Outdated Show resolved Hide resolved
far/editor.cpp Outdated Show resolved Hide resolved
far/editor.cpp Outdated Show resolved Hide resolved
far/editor.cpp Outdated Show resolved Hide resolved
far/editor.cpp Outdated Show resolved Hide resolved
far/findfile.cpp Show resolved Hide resolved
far/stddlg.cpp Outdated Show resolved Hide resolved
far/stddlg.cpp Outdated Show resolved Hide resolved
far/stddlg.cpp Outdated Show resolved Hide resolved
far/stddlg.hpp Outdated Show resolved Hide resolved
@MKadaner
Copy link
Contributor Author

It is rather massive so probably will take a few days.

Sure!

It is as small as I could make it. Originally, I wanted to use the dialog in Viewer as well, but then realized the change was already too big.

@MKadaner MKadaner force-pushed the mzk-stddlg-search branch 2 times, most recently from 82dc4fc to fad8ac5 Compare January 18, 2023 06:27
@rohitab
Copy link
Contributor

rohitab commented Jan 21, 2023

@MKadaner I just built and tested version 3.0.6093.0 (Private) x64 from MKadaner:mzk-stddlg-search

I tested out the changes you made and they seem to be working fine. The only issue I found so far is related to a hotkey in the Replace dialog of the Editor. If you press Alt+R, instead of clicking the Replace button, it inserts the word under the cursor in the Search for box. Basically, Alt+R works the same as Alt+O. In your test version, the one with Hex controls, pressing Alt+R selects the Text/Hex radio button.

You mentioned that Hex search is UI only and not implemented in Editor, so I'm not going to report any issues related to those. As an example, in the Replace dialog in the Editor, if you select Hex search, the Replace with box accepts text input, not hex input.

The focus issue #605 (comment) is still present in this release. However, it's not present in the new Hex control that you added in the Editor for testing. The cursor position issue mentioned in #607 is also present in both the Viewer and the new Hex control in the Editor. I'm not sure if you included a fix for that, so I'm reporting it just in case.

I did notice a few minor issues with Help and Hex search in Viewer, but they are present in the main Far branch, and are not related to your changes. I will open separate tickets for them to avoid any confusion.

Apologies for not being able to test sooner.

@MKadaner
Copy link
Contributor Author

@rohitab, Thank you so much for testing; incredibly helpful. Never mind the time, nobody's get paid here.

... issue ... related to a hotkey in the Replace dialog ...

This is a bug. I will have fixed it in the next iteration (will push in a few minutes).

The focus issue #605 (comment) ...

If it is in the Viewer, that is expected, I did not touch it. Frankly, I do not want to look there. I'd rather concentrate on replacing Viewer Search dialog with the unified one (next after this PR is merged), and the question will be moot.

The cursor position issue mentioned in #607 is also present in both the Viewer and the new Hex control in the Editor.

Yeah! This one is an issue on its own and is pretty much independent of the dialog behavior. Since it is not a regression, let's keep track of it in #607 and I will do something about it, probably after reworking Viewer Search.

... the Replace dialog in the Editor, if you select Hex search, the Replace with box accepts text input, not hex input.

For now, I just did not touch the Replace control, since in production it will never appear together with the Hex controls, not util we implement Hex search in Editor. And this one is interesting. When I try to think through my scenarios, I am not sure whether I want to replace a hex pattern with hex or text string. I need to actually use hex search in Editor to decide. Maybe the replace option should even be independent from the search one? Another interesting question is whether hex search in Editor should scan the decoded text or the underlying file, as in the Viewer. If it searches the file content, should it still search line by line, skipping line ends, as Editor does now?

I think we should start collecting requirements. @alabuzhev et al, does anybody have any suggestions? At any rate, hex search in Editor is relatively low on my priority list.

@alabuzhev
Copy link
Contributor

I think we should start collecting requirements. @alabuzhev et al, does anybody have any suggestions? At any rate, hex search in Editor is relatively low on my priority list.

Yes

image

I doubt that Hex search in Editor would make much sense - Editor does not work with bytes, so such mode would be quite hard to implement properly, and implementing it halfway would only confuse people.

@MKadaner
Copy link
Contributor Author

Yes

:) I've actually considered the "should". I remember using Far Editor several times to patch binary files like executables or images, even recently. It's possible if one is careful. I can imagine hex search / replace could be useful in such endeavors. But I agree that this feature is of very low priority, even if it's deemed useful.

@MKadaner MKadaner marked this pull request as ready for review January 22, 2023 17:37
@MKadaner
Copy link
Contributor Author

Removed test scaffolding, changelog, version, rebased, squashed, ready.

Thank you for the review!

@alabuzhev
Copy link
Contributor

@MKadaner I've pushed a commit before noticing your latest changes, please rebase again.

@MKadaner
Copy link
Contributor Author

@alabuzhev Done!

@alabuzhev alabuzhev merged commit 8891d36 into FarGroup:master Jan 23, 2023
@alabuzhev
Copy link
Contributor

Thank you

@MKadaner MKadaner deleted the mzk-stddlg-search branch January 23, 2023 03:25
@rohitab
Copy link
Contributor

rohitab commented Jan 26, 2023

Editor Replace Button Hotkey

Most dialogs in Far do not have hotkeys for the default button, since you can just press Enter. For example, in the Search dialog in Viewer/Editor/Help, the Search button does not have a hotkey. However, in the Replace dialog in the Editor, the Replace button has an Alt+R hotkey.

What do you think about removing the hotkey from the Replace button and assigning Alt+R to the Replace with edit control instead? It seems natural to me, to use Alt+S and Alt+R to switch between Search and Replace with boxes. It might inconvenience people that are accustomed to using the Alt+R/Alt+E hotkeys though.

@MKadaner
Copy link
Contributor Author

I also noticed this discrepancy. I think this is a good idea. The decision is mostly for the language maintainers, except for there is no dedicated maintainer for English localization.

P.S. maybe move this discussion to, well, "Discussions?"

@rohitab
Copy link
Contributor

rohitab commented Jan 27, 2023

Moved to Discussions: #616

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.

None yet

3 participants