Skip to content

Allow copying Shift JIS encoded string literals #189

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

Merged
merged 3 commits into from
Apr 17, 2025

Conversation

LagoLunatic
Copy link
Contributor

Implements decoding string literals as Shift JIS in addition to ASCII. This works with both pooled and unpooled (#188) strings.

Note that I specify "copying" Shift JIS strings and not "displaying" them, because as far as know, egui does not support fallback fonts. So if the current font doesn't support Japanese characters (which objdiff's default font doesn't), then egui will simply display them as boxes rather than fallback to a system font that displays them properly:

image

The characters can be displayed if the user manually switches their Code font in the settings to one that has Japanese characters, but note that this changes the font for all the code, not just these strings, so it may or may not be desirable:

image

Either way, being able to just copy the strings from objdiff is more important than being able to read them.

@encounter
Copy link
Owner

I wish the strings could be displayed in the GUI! But I do agree that copying is more important than being able to read them.

In both the tooltip and context menu, it'd be great to display "Shift JIS" to disambiguate it from the regular string.

@LagoLunatic
Copy link
Contributor Author

In both the tooltip and context menu, it'd be great to display "Shift JIS" to disambiguate it from the regular string.

The text on the left ("String:") is currently tied only to the data type so I can't have the first result show "String:" and the second show "Shift JIS:".

Do you mean that each string encoding should be a separate data type, so e.g. we'd split "String" into "ASCII" "Shift JIS" etc, and only display a single result with the correct encoding, getting rid of the "\x90X\x82..." gibberish option from trying to show non-ASCII strings as ASCII? I feel like that could maybe cause issues in the future if objdiff guesses the wrong encoding and refuses to show alternate encoding options, maybe not very likely with just ASCII and ShiftJIS but if more encodings are added later it could become more likely.

Or do you mean the label text should be based on the both type and the value, not just the type? I'm not sure what the cleanest way to code that would be. Should display_literals return extra info about the value in addition to the string itself, like what encoding was used? (e.g. Vec of tuples of String+enum.) Then display_labels could use that info to change the label conditionally maybe.

@encounter
Copy link
Owner

I’m not super opinionated on how it’s achieved, but I definitely do think there should be just some indication in both the tooltip and context menu what encoding it is. I don’t think splitting it into a separate DataType is necessary, Returning a tuple from display_literals seems fine.

@LagoLunatic
Copy link
Contributor Author

image

image

@encounter
Copy link
Owner

Perfect, thank you!

@encounter encounter merged commit b40fae5 into encounter:main Apr 17, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants