-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add memory visualization #321
base: main
Are you sure you want to change the base?
Conversation
I see that some of the errors come from using a value that doesn't exist yet, which often boils down to finding the width or height of some component before it's rendered. This has lead to a few "1 frame latency" problems:
Is there some way to render the elements twice and use their first width for the second render to avoid this problem? It gives an error in the editor, but not even a warning in the dev console when running in the app. |
iconName="mdi mdi-folder-open" | ||
title="Memory visualization" | ||
description="Drag & drop HEX/ZIP/ELF files here" | ||
iconName="mdi mdi-file-outline" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for changing the title and icon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think it's more intuitive that you're looking at an elf-file if there's a file icon + extension, than if you just see a directory/folder icon. But the title change should be reverted, thanks for catching that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is right now it changes the default behaviour so that mdi-file-outline is shown also for all other states (like no files or other file types).
Do ask Ketil though, maybe he thinks this is how it should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1337
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will work on this so it works with hex and zip files too
src/components/Canvas.tsx
Outdated
|
||
const Canvas = () => { | ||
const canvasRef = useRef<HTMLCanvasElement>(null); | ||
const [c, setContext] = useState<CanvasRenderingContext2D | null>(null); // Why could this be |null ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of places in this file where "c" (the canvas reference) and its properties could be null because it's not rendered yet, which is a common problem here. See my comment in the PR for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at feedback from Pascal, reverted some unneeded changes.
Since most of the "1 frame delay" problems are just visual, I have mainly focused on trying to keep the element width fixed, as that solves a related "update oscillator" crashing problem. Tried doing it in code first, but just keeping the scrollbar visible at all times seemed to be simplest. I asked Ketil how he prefers the scrollbar layout, though the compact (2nd image) solution is a bit more tricky to implement because it needs to overwrite a react bootstrap (edit: or shared?) Card property, but I'm sure I'll figure it out 😃 I've also been working on trying to make the tooltip update when you scroll (and don't move the mouse), but it seems like it's a bit complicated given that they're two different event signatures. |
Some thoughts regarding colors in the visualization: Currently they are just arbitrary (but deterministic) colors, but i guess it's kind of useful because it makes it clear when you're looking at a different file (because the largest sections will often have different colors, and it's easy to see which section is currently selected. Would it be preferable for the colors to indicate the semantics of each section, like "this section is debug stuff, therefore it's red"? This would make it more difficult to distinguish between sections close to each other, which are often similar. I know these are mostly Ketil-questions, but I'm just putting them out here to get ideas or suggestions on what you'd prefer. |
addMruFile(filePath); | ||
// License for elfy can be found at https://github.com/indutny/elfy | ||
/* eslint-disable global-require */ | ||
const elfy = require('elfy'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a reason for using an inline require instead of a global import statement.
122236f
to
4c952ca
Compare
Code is copied from this repo, with a few manual changes, mostly just renaming from "memory-visualization" to "programmer" and a few dependencies for reading elf-files.