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

Implement the hex editor widget for Cocoa and enable memory editor on macOS #1723

Merged
merged 4 commits into from
Dec 17, 2024

Conversation

Cathrach
Copy link
Contributor

This PR adds the hex editor widget to Cocoa hiro and enables the memory editor in Tools. The widget is a cell-based NSTableView with data source the core hex editor widget and supports editing of memory (but not of the encoded ANSI).

…t. Squashed following commits:

First try: cocoahexedit is view, source, and delegate. IDK if it compiles. No updates.

I...think it compiles?

I...think it compiles?

trying to separate tableview out like in table_view.cpp

Attempt to set document view to table view so table shows (still not working)

IT DISPLAYS A BLANK TABLE NOW

IT DISPLAYS THINGS! YAY

changed to objectValueForTableColumn to show text.

Separate columns (with all spacing removed so they fit) in prep for editing

Somehow getting "CLIENT ERROR: TUINSRemoteViewController does not override -viewServiceDidTerminateWithError: and thus cannot react to catastrophic errors beyond logging them" on any kind of editing...

Tried emptying function and removing delegate/target stuff so data source only. No dice.

try simplest thing (always replace with 0) - not working. :(

Corrected the typo (thanks yam). Editing works now.

Reformat code

Add some documentation tehe

Make sure we don't read bytes past the length! oops.
@Cathrach Cathrach marked this pull request as ready for review December 15, 2024 17:24
auto pHexEdit::destruct() -> void {
[cocoaView removeFromSuperview];
- (NSInteger) numberOfRowsInTableView:(NSTableView *)tableView {
return hexEdit->rows();
Copy link
Contributor

@jcm93 jcm93 Dec 15, 2024

Choose a reason for hiding this comment

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

Hmm; it looks like in other hex-edit implementations, rows is being dynamically set based on the amount of memory being looked at (hexEdit->length()). For Cocoa here, we aren't currently doing that, so this value appears to always remain at 16. I think we need to derive the number of rows from hexEdit->length in this method, or else set hexEdit's rows somewhere else based on the total memory length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you – I was misunderstanding the functionality. Newest commit fixes this so rows = length / columns and properly implements address goto!

Copy link
Contributor

@jcm93 jcm93 left a comment

Choose a reason for hiding this comment

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

Thanks very much for the work here! One other general comment is that we may want a slightly larger gap between the last hex column and the column showing the encoded text, just so the separation is a bit more apparent.

if (address < hexEdit->length()) {
u8 data = hexEdit->doRead(address++);
[output appendString:[NSString stringWithFormat:@"%c",
(data >= 0x20 && data <= 0x7e ? (char)data : '.')]];
Copy link
Contributor

@jcm93 jcm93 Dec 15, 2024

Choose a reason for hiding this comment

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

This is something of a nit/bikeshedding, since the terminology isn't exposed to the user currently, but "ANSI" is sort of a Windows-ism, which just sort of generically means "the system's default encoding". On macOS, that term isn't really used, since basically everything is UTF-8. It looks like when we're interpolating the char here, we're using the UTF-8 system-default interpolation of the character, but just constrained to the range 32-127 of printable characters that have only one codepoint. Not worth changing the names of everything (unless you feel like it), but if the column header is ever exposed to the user, worth keeping in mind.

I also personally would prefer a whitespace instead of a ., since the actual . character is within the 32-127 range, but others might have other opinions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks – I changed to a "Plain text" header. The . is for consistency with the windows/gtk versions; also, I think whitespace is difficult to count.

hiro/cocoa/widget/hex-edit.cpp Outdated Show resolved Hide resolved
hiro/cocoa/widget/hex-edit.cpp Show resolved Hide resolved
NSTextFieldCell *cell = column.dataCell;

// Set the font for the data cell.
cell.font = [NSFont monospacedSystemFontOfSize:10 weight:NSFontWeightRegular];
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also only available on macOS 10.15 or above, so we should guard it with an availability check:

Suggested change
cell.font = [NSFont monospacedSystemFontOfSize:10 weight:NSFontWeightRegular];
if (@available(macOS 10.15, *)) {
cell.font = [NSFont monospacedSystemFontOfSize:10 weight:NSFontWeightRegular];
}

I forget how onerous implementing other fonts for cells below macOS 10.15 is; if it's annoying, I wouldn't worry too much about it. You may, however, need to widen the hex columns if the monospace font isn't available.

@jcm93
Copy link
Contributor

jcm93 commented Dec 15, 2024

One other thing; the memory editor is also current ifdef'd out in the menu bar's Tools menu;

// Cocoa hiro is missing the hex editor widget

Should remove that in the header there as well as the implementation file.

Copy link
Contributor

@jcm93 jcm93 left a comment

Choose a reason for hiding this comment

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

Looking good! Just a couple more style suggestions / nits.

hiro/cocoa/widget/hex-edit.cpp Show resolved Hide resolved
hiro/cocoa/widget/hex-edit.cpp Outdated Show resolved Hide resolved
hiro/cocoa/widget/hex-edit.cpp Outdated Show resolved Hide resolved
Co-authored-by: jcm <6864788+jcm93@users.noreply.github.com>
@jcm93
Copy link
Contributor

jcm93 commented Dec 16, 2024

Looks good to me! If two-way editing is a part of the GTK/Windows version, I think it can be worried about later, this seems plenty good for a first iteration to me at least.

@LukeUsher LukeUsher merged commit 7d327a2 into ares-emulator:master Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants