RfidResearchGroup / proxmark3

Iceman Fork - Proxmark3
http://www.icedev.se
GNU General Public License v3.0
3.62k stars 979 forks source link

Plot Improvements, Round 1 #2345

Closed jlitewski closed 2 months ago

jlitewski commented 2 months ago

This is the first round of changes to the Plot Window I'm making to better support some tools I'm hacking together to help with my ventures.

Firstly, I implemented the first parts of the Operation Buffer. The Operation Buffer is going to be the buffer that the various other commands modify data on, which then gets shown as a citron-colored line on the graph window. This will serve as a visual representation of the proposed changes before they get applied to the Graph Buffer (which still needs to have the logic implemented to do so), so if a command didn't do what you wanted it to, or if there was a mistake, it wouldn't trash the data on the Graph Buffer, needing you to reload the trace and try again. Currently, none of the commands use this functionality yet, but as I flesh it out, I'll move them over to using it.

Secondly, I tweaked the cursor movement via the hot keys. If the cursor moves off either side of the plot window, it'll recenter on the cursor. More of a QOL change than anything. The manual tweaking of values will be next, having it default to tweaking data on the Operation Buffer normally and tweaking data on the Graph and Operation Buffer while holding shift.

Lastly, I rearranged the commands in cmddata.c. Truthfully, it annoyed me how scattered everything felt. The data clear command does have a different tip currently as I was in the middle of modifying that command first for the Operation Buffer, but I thought it would be good to get these changes pushed upstream before that work is done.

github-actions[bot] commented 2 months ago

You are welcome to add an entry to the CHANGELOG.md as well

jlitewski commented 2 months ago

I love the ability to select certain things I want committed in VSCode, but I hate when it decides to ignore me. Amended the previous commit, it should build correctly now.

iceman1001 commented 2 months ago

Interesting,

Try not to fall into the hole of "I will just add one more thing to this PR" and you end up with lots of irrelevant commits making the review process so much harder.

In this case, the rearrangement of the data command list is... confusing. it is not alphabetic with in their sub groups. So, I will not merge this PR for now.

The look and feel for some people is not for everyone. like opinions everyone got one. So please refrain from thinking your style applies to me and this project. I do agree that the client isn't perfect in any way but we try to follow a certain feeling for how it should be.

Make small PR's those are so much simpler to review.
Major rehauls would need to be discussed before either here or on the discord proxmark3-dev channel.

jlitewski commented 2 months ago

The reordering of the commands was more putting like commands together (ltrim, rtrim, etc) and putting the generic commands (save, load, clear, etc) up top with the help command. It felt more organized this way.

As for the multiple commits, I'll keep that in mind for the future.

jlitewski commented 2 months ago

Ffs, hit the wrong button

jlitewski commented 2 months ago

Amended the commit to have the cmddata.c commands in alphabetical order

iceman1001 commented 2 months ago

Thank you for understanding.