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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
082db12
Remove deprecated/invalid JetBrains IDE configs
AdmiralPotato Jul 4, 2020
8578958
Fix: Arrow keys in HexEditor update InfoPanel
AdmiralPotato Jul 4, 2020
076e69c
Shift + Arrow keys in HexEditor now select range
AdmiralPotato Jul 4, 2020
79a3057
Shift + Click in HexEditor now selects range
AdmiralPotato Jul 4, 2020
3fe5762
Improve HexEditor Shift logic, Add cursor display
AdmiralPotato Jul 6, 2020
c01f942
HexViewer: fix formatting, remove unneded string interpolation
generalmimon Nov 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,4 +254,8 @@ lib/_npm/
*.pyc
.serve_files_secret
\.DS_Store
playground/*.js*
playground/*.js*

# Exclude JetBrains IDE files
*.iml
.idea
6 changes: 0 additions & 6 deletions .idea/misc.xml
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions .idea/modules.xml
View file
Open in desktop

This file was deleted.

9 changes: 0 additions & 9 deletions .idea/typescript-compiler.xml
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions .idea/vcs.xml
View file
Open in desktop

This file was deleted.

12 changes: 0 additions & 12 deletions .idea/webide.iml
View file
Open in desktop

This file was deleted.

394 changes: 0 additions & 394 deletions .idea/workspace.xml
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions css/HexViewer.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
.hexViewer .header .hex .c8 { margin-left: 17px }

.hexViewer .selected { background:#0076e0; color:white }
.hexViewer .cursor { background: #00ccff; }
.hexViewer .asciicell { padding: 2px 0 }

.hexViewer .hexRow { height:21px; }
Expand Down
85 changes: 65 additions & 20 deletions src/HexViewer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,10 @@ export class HexViewer {
private content: JQuery;
private contentOuter: JQuery;

public mouseDownOffset: number;
private canDeselect: boolean;

public selectionStart: number = -1;
public selectionEnd: number = -1;
public selectionCursor: number = -1;
Copy link
Member

@generalmimon generalmimon Jul 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

public onSelectionChanged: (() => void);

private isRecursive: boolean;
Expand All @@ -125,15 +124,18 @@ export class HexViewer {
if ("dataOffset" in cell) {
if (e.type === "mousedown") {
this.canDeselect = this.selectionStart === cell.dataOffset && this.selectionEnd === cell.dataOffset;
this.mouseDownOffset = cell.dataOffset;
this.content.on("mousemove", evt => this.cellMouseAction(evt));
this.setSelection(cell.dataOffset, cell.dataOffset);
if (e.shiftKey) {
this.shiftCursor(cell.dataOffset);
} else {
this.setCursor(cell.dataOffset);
}
}
else if (e.type === "mousemove") {
this.setSelection(this.mouseDownOffset, cell.dataOffset);
this.shiftCursor(cell.dataOffset);
this.canDeselect = false;
}
else if (e.type === "mouseup" && this.canDeselect && this.mouseDownOffset === cell.dataOffset)
else if (e.type === "mouseup" && this.canDeselect && this.selectionStart === cell.dataOffset)
this.deselect();

this.contentOuter.focus();
Expand Down Expand Up @@ -177,12 +179,12 @@ export class HexViewer {
e.key === "ArrowRight" ? 1 : e.key === "ArrowLeft" ? -1 : null;

if (selDiff === null) return;

var newSel = this.selectionStart + selDiff;
if (newSel < 0) newSel = 0;
else if (newSel >= this.dataProvider.length) newSel = this.dataProvider.length - 1;
Comment on lines -182 to -183
Copy link
Member

@generalmimon generalmimon Jul 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

kaitai-struct-webide-mouse-click-outside

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.


this.setSelection(newSel);
var newSel = this.selectionCursor + selDiff;
if (e.shiftKey) {
this.shiftCursor(newSel);
} else {
this.setCursor(newSel);
}
return false;
});
}
Expand Down Expand Up @@ -256,17 +258,27 @@ export class HexViewer {
hexCell.cell.dataOffset = asciiCell.dataOffset = dataOffset;

var isSelected = this.selectionStart <= dataOffset && dataOffset <= this.selectionEnd;
$(hexCell.cell).toggleClass("selected", isSelected);
$(asciiCell).toggleClass("selected", isSelected);
var isCursor = dataOffset === this.selectionCursor;
var targets = [hexCell.cell, asciiCell];
$(targets).toggleClass("selected", isSelected);
$(targets).toggleClass("cursor", isCursor);

var skipInt = 0;
for (var level = 0; level < this.maxLevel; level++) {
var int = intervals[intIdx + level];
var intIn = int && int.start <= dataOffset && dataOffset <= int.end;
var intStart = intIn && int.start === dataOffset;
var intEnd = intIn && int.end === dataOffset;
hexCell.levels[level].className = `l${this.maxLevel - 1 - level} ${((intBaseIdx + intIdx) % 2 === 0) ? "even" : "odd"}` +
(intIn ? ` m${level}` : "") + (intStart ? " start" : "") + (intEnd ? " end" : "") + (isSelected ? " selected" : "");
var classes = [
`l${this.maxLevel - 1 - level}`,
`${((intBaseIdx + intIdx) % 2 === 0) ? "even" : "odd"}`,
generalmimon marked this conversation as resolved.
Show resolved Hide resolved
];
if(intIn) classes.push(`m${level}`);
if(intStart) classes.push("start");
if(intEnd) classes.push("end");
if(isSelected) classes.push("selected");
if(isCursor) classes.push("cursor");
hexCell.levels[level].className = classes.join(" ");

if (intEnd)
skipInt++;
Expand All @@ -291,16 +303,49 @@ export class HexViewer {
}

public deselect() {
this.setSelection(-1, -1);
this.setCursor(-1);
}

public setSelection(start: number, end?: number) {
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
);
}
Comment on lines +309 to +329
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public setSelection(start: number, end?: number, cursor?: number) {
if (this.isRecursive) return;
end = end || start;

var oldStart = this.selectionStart, oldEnd = this.selectionEnd;
this.selectionStart = start < end ? start : end;
this.selectionEnd = Math.min(start < end ? end : start, this.dataProvider.length - 1);
this.selectionStart = Math.min(start, end);
this.selectionEnd = Math.min(
Math.max(start, end),
this.dataProvider.length - 1
);

// 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;
Comment on lines +342 to +347
Copy link
Member

@generalmimon generalmimon Jul 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (this.selectionStart !== oldStart || this.selectionEnd !== oldEnd) {
this.isRecursive = true;
try {
Expand Down
14 changes: 11 additions & 3 deletions src/v1/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,15 +207,19 @@ class AppController {

onHexViewerSelectionChanged() {
//console.log("setSelection", ui.hexViewer.selectionStart, ui.hexViewer.selectionEnd);
localStorage.setItem("selection", JSON.stringify({ start: this.ui.hexViewer.selectionStart, end: this.ui.hexViewer.selectionEnd }));
localStorage.setItem("selection", JSON.stringify({
start: this.ui.hexViewer.selectionStart,
end: this.ui.hexViewer.selectionEnd,
cursor: this.ui.hexViewer.selectionCursor
}));

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);
Copy link
Member

@generalmimon generalmimon Jul 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (intervals.items.length > 0) {
//console.log("selected node", intervals[0].id);
this.blockRecursive = true;
Expand Down Expand Up @@ -281,7 +285,11 @@ $(() => {
app.inputReady.then(() => {
var storedSelection = JSON.parse(localStorage.getItem("selection"));
if (storedSelection)
app.ui.hexViewer.setSelection(storedSelection.start, storedSelection.end);
app.ui.hexViewer.setSelection(
storedSelection.start,
storedSelection.end,
storedSelection.cursor
);
});

var editDelay = new Delayed(500);
Expand Down