Shift + (Click/Arrows) in HexEditor to select multiple bytes #119

AdmiralPotato · 2020-07-04T13:15:14Z

I desired the ability to select multiple bytes in the HexEditor using the Shift key in much the same way as one would make text selections in many other text editors, so I added those capabilities.

This change set allows the selection of multiple bytes in the HexEditor in these two scenarios:

Shift+Clicking on a cell selects all bytes in between the target cell and the last text cursor position
Holding Shift and using the Arrow keys adds or removes bytes from the selection, relative to the last text cursor position

This change also fixes a bug where clicking on bytes in the HexEditor updated the the InfoPanel's Selected: $PATH label, but that label was not updated when using the Arrow keys to change the position of the selectionStart text cursor.

Some outdated JetBrains IDE configuration files were also removed, as they were causing errors in newer versions of JetBrains tools.

generalmimon · 2020-07-04T15:48:30Z

@AdmiralPotato For your own convenience, remember a rule of thumb when making pull requests from your fork repository: never commit directly on master of your fork. Read kaitai-io/kaitai_struct_doc#38 (comment) for more info.

src/HexViewer.ts

AdmiralPotato · 2020-07-06T06:18:51Z

@generalmimon Thank you for the suggestion to make future pull requests from a dedicated branch, I will keep that workflow in mind the next time I make one.

After some careful analysis of your suggestions, I took the time to revise my approach; both to address your concerns, as well as to add the feature that other editors have used to help illustrate the current cursor position - a slight color difference on the cell where the "cursor" currently rests.

Do these updated changes meet your criteria?

generalmimon · 2020-07-25T13:47:36Z

Some outdated JetBrains IDE configuration files were also removed, as they were causing errors in newer versions of JetBrains tools.

Next time please open a special pull request for each topic / related group of changes, it's not good to mix completely unrelated changes in one PR. See The Art of Small Pull Requests.

generalmimon · 2020-07-25T14:00:04Z

src/HexViewer.ts

    public selectionStart: number = -1;
    public selectionEnd: number = -1;
+    public selectionCursor: number = -1;


Again, I think that the special variable for selectionCursor is unnecessary, because always applies either selectionCursor == selectionStart or selectionCursor == selectionEnd, nothing between or outside. Why not introduce a single property isSelectionCursorAtEnd which just distinguishes between these two states (instead of the selectionCursor)?

generalmimon · 2020-07-25T15:00:51Z

src/HexViewer.ts

+        // is cursor out of bounds because called externally?
+        if (
+            this.selectionCursor < this.selectionStart
+            || this.selectionCursor > this.selectionEnd
+        ) cursor = this.selectionEnd;
+        if (cursor) this.selectionCursor = cursor;


A weird condition. I don't fully understand the case if (this.selectionCursor < this.selectionStart) { cursor = this.selectionEnd }, shouldn't it be cursor = this.selectionStart for proper clamping?

Your solution +---------------------+ | | | v .*.........|...........|......... ^ ^ ^ cursor start end Clamping +---------+ | | | v .*.........|...........|......... ^ ^ ^ cursor start end

Anyway, clamping feels weird when you consider that the situations above should never happen, because ideally cursor === this.selectionStart || cursor === this.selectionEnd applies all the time. Speaking of which, your condition doesn't handle the case cursor > this.selectionStart && cursor < this.selectionEnd at all.

In any case, this is the result of storing the selectionCursor separately. You wouldn't have to handle these states of inconsistency if you weren't storing the same data in different variables.

src/HexViewer.ts

generalmimon · 2020-07-25T18:23:31Z

src/HexViewer.ts

-            if (newSel < 0) newSel = 0;
-            else if (newSel >= this.dataProvider.length) newSel = this.dataProvider.length - 1;


These indispensable checks are removed without any compensation. As a result, when you try to seek before the beginning using the keyboard keys (place the cursor somewhere on the first line of the hex dump and then press the Up Key ↑), some weird stuff happens and soon the hex dump becomes completely unusable. Trying to interact with the hex dump using the keyboard or the mouse doesn't do anything. AFAIK the only way how to get things back to normal is to reload the page.

So please at least bring them back so that this new implementation isn't broken more that the existing one.

To be fair, this adjustment apparently solves the problem with the keyboard, but a similar problem persists using the mouse (clicking on some invisible "byte" below the hex dump), though it's much smaller bug. Here's the capture:

It would be nice if you could solve it as well, preferably by moving this check that you removed here to some place common for both keyboard and mouse interaction, for example the setSelection method. There are already some checks, but they apparently aren't enough.

generalmimon · 2020-07-25T18:31:11Z

src/HexViewer.ts

+    public setCursor (cursor: number) {
+        this.selectionCursor = cursor;
+        this.setSelection(
+            this.selectionCursor
+        );
+    }
+
+    public shiftCursor (cursor: number) {
+        var start = this.selectionStart;
+        var end = this.selectionEnd;
+        if (this.selectionCursor === end) {
+            end = cursor;
+        } else if (this.selectionCursor === start) {
+            start = cursor;
+        }
+        this.selectionCursor = cursor;
+        this.setSelection(
+            Math.max(0, start),
+            end
+        );
+    }


Hopefully these methods won't be needed at all when you eliminate the selectionCursor variable (#119 (comment)).

generalmimon · 2020-07-25T18:38:46Z

src/v1/app.ts


        var start = this.ui.hexViewer.selectionStart;
        var hasSelection = start !== -1;

        this.refreshSelectionInput();

        if (this.ui.parsedDataTreeHandler && hasSelection && !this.selectedInTree) {
-            var intervals = this.ui.parsedDataTreeHandler.intervalHandler.searchRange(this.ui.hexViewer.mouseDownOffset || start);
+            var intervals = this.ui.parsedDataTreeHandler.intervalHandler.searchRange(this.ui.hexViewer.selectionCursor || start);


Makes perfect sense, thanks! 👍

One currently has to click on some byte using mouse in order to highlight the relevant field in the object tree, so indeed, the keyboard control of the Web IDE is quite limited.

To be clear, I'm still convinced that selectionCursor variable is unnecessary and something like this.ui.hexViewer.isSelectionCursorAtEnd ? this.ui.hexViewer.selectionEnd : this.ui.hexViewer.selectionStart would do better. I just like the idea.

AdmiralPotato added 4 commits July 4, 2020 03:51

Remove deprecated/invalid JetBrains IDE configs

082db12

Fix: Arrow keys in HexEditor update InfoPanel

8578958

Shift + Arrow keys in HexEditor now select range

076e69c

Shift + Click in HexEditor now selects range

79a3057

generalmimon requested review from koczkatamas, GreyCat and generalmimon July 4, 2020 15:42

generalmimon reviewed Jul 4, 2020

View reviewed changes

src/HexViewer.ts Outdated Show resolved Hide resolved

generalmimon reviewed Jul 4, 2020

View reviewed changes

src/HexViewer.ts Outdated Show resolved Hide resolved

generalmimon reviewed Jul 4, 2020

View reviewed changes

src/HexViewer.ts Outdated Show resolved Hide resolved

generalmimon added the new feature label Jul 5, 2020

Improve HexEditor Shift logic, Add cursor display

3fe5762

AdmiralPotato requested a review from generalmimon July 7, 2020 22:59

generalmimon reviewed Jul 25, 2020

View reviewed changes

src/HexViewer.ts Outdated Show resolved Hide resolved

generalmimon reviewed Jul 25, 2020

View reviewed changes

generalmimon added the enhancement label Jul 26, 2020

HexViewer: fix formatting, remove unneded string interpolation

c01f942

generalmimon force-pushed the master branch from 2626ec5 to c01f942 Compare March 27, 2022 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shift + (Click/Arrows) in HexEditor to select multiple bytes #119

Shift + (Click/Arrows) in HexEditor to select multiple bytes #119

AdmiralPotato commented Jul 4, 2020

generalmimon commented Jul 4, 2020

AdmiralPotato commented Jul 6, 2020

generalmimon commented Jul 25, 2020

generalmimon Jul 25, 2020 •

edited

Loading

generalmimon Jul 25, 2020 •

edited

Loading

generalmimon Jul 25, 2020 •

edited

Loading

generalmimon Jul 25, 2020

generalmimon Jul 25, 2020 •

edited

Loading

		if (newSel < 0) newSel = 0;
		else if (newSel >= this.dataProvider.length) newSel = this.dataProvider.length - 1;

Shift + (Click/Arrows) in HexEditor to select multiple bytes #119

Are you sure you want to change the base?

Shift + (Click/Arrows) in HexEditor to select multiple bytes #119

Conversation

AdmiralPotato commented Jul 4, 2020

generalmimon commented Jul 4, 2020

AdmiralPotato commented Jul 6, 2020

generalmimon commented Jul 25, 2020

generalmimon Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

generalmimon Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

generalmimon Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

generalmimon Jul 25, 2020

Choose a reason for hiding this comment

generalmimon Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

generalmimon Jul 25, 2020 •

edited

Loading

generalmimon Jul 25, 2020 •

edited

Loading

generalmimon Jul 25, 2020 •

edited

Loading

generalmimon Jul 25, 2020 •

edited

Loading