-
Notifications
You must be signed in to change notification settings - Fork 632
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
Apply hex cleaning to search dialog paste operations #319
Conversation
75d647c
to
d697c2b
Compare
Converted to draft, I'm just trying to apply the same update to Hex pattern as well, as it seems useful there too. |
d697c2b
to
51fd2c1
Compare
I have cleaned hex pasting into "Hex pattern" as well, working in Win32 in https://github.com/mikebeaton/UEFITool/tree/try-fix , and it is indeed nicer to use, but it is failing to build in Win64/MinGW - just in case anybody can spot how to resolve the qt6-specific issue there before I can. (It's not liking the class inheritance hierarchy.) |
- Permits pasting to 'GUID' search directly from cpp representation - Provides hex cleaning (e.g. auto-remove 0x) in 'Hex pattern' search as well
Thank you for the PR, but I seriously doubt this feature is needed in the mainline UEFITool. I'm personally not a fan of such kind of "automated magic tricks" where you paste something and it magically changes from its original format. GUIDs are formatted as 8-4-4-2-6 and the fact that EDK2 uses a different format is a limitation of C, not a limitation of UEFITool, and the problem needs to be resolved on their part, not ours, not to mention the feature clashing with "any hex digit of a pattern or GUID can be replaced with a '.' to mean 'any'". Clarity (as in "a program behaves in a way one expects it to") is more important for me than convenience, so I will not be accepting this PR. I do not want to discourage you from further contributions, but please do open an issue where we can discuss the problem and its possible solutions before writing code, so the effort of writing it won't be lost because of maintainer disagreements. CC @vit9696 for his opinion. |
51fd2c1
to
3fdc4fe
Compare
I finished the feature at least as I intended, so for now, at least, have marked it back as ready to merge - in principle. Thanks for the review. It's probably not worth 'arguing' - of course, as it is your own extremely useful project! But I was trying to use the tool 'in earnest' for the first time, over the last few days - actually in order to try to finally make the MacPro5,1 Apple firmware boot picker application start, on an unsupported card, once OpenCore has set up the various graphics protocols which should be all that it requires. It currently won't, for reasons which have eluded my and others' investigations so far. The details of the project don't matter, except that this is a typical (I would have thought) workflow where multiple times I was trying to find whether a given Apple or EDK-II GUID was in the firmware, and if so in which section, so multiple times I was having to manually clean each GUID before I could search for it. I felt (but perhaps wrongly) that this must be a pain point that other users are feeling. At least I can say that it did (does) make that workflow far more convenient. FWIW. |
Apologies for not opening an issue first to discuss before flinging some code at you, though! |
It is definitely a pain point for others, and I'm arguing about making a more intuitive implementation (akin of "Paste from..." that will make format conversion both visible and user-triggered instead of automagical), not ditching it entirely. As @vit9696 correctly said in a side discussion, we all use UEFITool for slightly different workflows, all of such workflows are valid, and we need to somewhat support all of them, even if neither me nor him are exercising some of them. However, I'm trying my best to limit the feature and scope creep for the tool, and allow for alternative solutions that do not involve us making the tool new IDA, a new 010Editor or a new Nero Burning Rom. It is after all a hobby project supported by people very busy at their day jobs already. Let's go discuss this in a "Search sucks" kind of issue, I'll leave the PR open as an inspiration and reminder that the original problem is still unresolved. |
Let's continue the discussion in #320 |
To add a few more comments, I personally believe that the feature is very useful, as copying the GUID from C code and then trying to find it in the UEFITool for me is a regular workflow when reversing UEFI code. However, it really worries me to have that done transparently for the end user as it may eventually cause minor issues: endianness swapping, parsing problems due to handling inline comments with latin characters, not handling GUIDs with In my opinion, what this definitely needs is |
EDK-II and other code is full of representations of GUIDs such as
{ 0xBB25CF6F, 0xF1D4, 0x11D2, { 0x9A, 0x0C, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0xFD }}
.It can be quite inconvenient to have to manually clean these into the format
BB25CF6FF1D411D29A0C0090273FC1FD
before pasting into UEFITool's GUID search function each time. This pull request does the required cleaning automatically, so you can copy all of something like the above and then paste it directly into the GUID field, getting the desired result, which I believe should be much more convenient in daily use.Due to limitations on hooking paste in
QLineEdit
, we lose any non-plain text formatting of whatever was in the clipboard before this operation (which would not normally happen when pasting formatted text into a plain text application), but I believe this should not be a major issue.