Make memory address padding configurable #89
Conversation
| fetchMemory={this.props.fetchMemory} | ||
| isMemoryFetching={this.props.isMemoryFetching} | ||
| scrollingBehavior={this.props.scrollingBehavior} | ||
| addressPadding={this.props.addressPadding} |
There was a problem hiding this comment.
I have some misgivings about the number of places we're passing around the new memoized padding / length values when they're relevant to only one column, and we should avoid knowing too much about column internals outside of those contributions.
One idea about how to handle it would be to introduce a new lifecycle on the columns so that they can update any relevant state onComponentDidUpdate or whatever seems best. Another (obviously beyond the scope of this PR) would be to use something other than React so that individual columns could have tighter control of their own rendering.
There was a problem hiding this comment.
Do you mean -- conceptually -- onComponentDidUpdate in App, we invoke an update on each column, passing them a new object state to avoid having to pass column-specific state down to the MemoryWidget/MemoryTable?
That sounds like a good idea to me!
I feel though such a change probably deserves a separate PR, wdyt?
There was a problem hiding this comment.
Do you mean -- conceptually -- onComponentDidUpdate in App, we invoke an update on each column, passing them a new object state to avoid having to pass column-specific state down to the MemoryWidget/MemoryTable?
Yes, exactly. That way columns are kept up-to-date on changes in props/state and can store the results of any calculations they'd otherwise repeat.
I feel though such a change probably deserves a separate PR, wdyt?
I'd be inclined to do it here because this PR is introducing a new prop that's being passed in a style we might like to avoid, but if you feel strongly, we could put it off.
There was a problem hiding this comment.
To be frank: I agree with @planger that this should be addressed in a separate refactoring PR.
It is a great idea to have more fine granular control over updates. In the sense of avoiding to flood components with "unsed" or just passed through props. But it sounds like this is something that should be properly analyzed and rolled out across the board. I am sure there is more potential for this beyond the address padding. And a separate PR will give clearer separation of the feature enhancement and refactoring work.
There was a problem hiding this comment.
I'm ok with that approach, though I think it's not generally a good idea to acknowledge that something is not the direction we want to go and then move forward with it anyway. I've approved the PR - @planger, feel free to make whatever changes to the wording of the preference enum descriptions that you prefer and merge at your convenience.
There was a problem hiding this comment.
Thanks, @colin-grant-work ! I appreciate that the situation isn't ideal. Let's make sure that we address this as soon as we can.
|
@colin-grant-work Thank you very much for your review and raising your ideas for improvements! I commented on them above, but would prefer to address those general improvements in separate PRs, unless you feel otherwise. |
package.json
Outdated
| ], | ||
| "default": "Minimal", | ||
| "enumDescriptions": [ | ||
| "Minimal padding based on the longest address in range.", |
There was a problem hiding this comment.
Just a few suggestions for the wording:
Pads address values with leading zeroes to largest address in loaded range.
Disables padding of address values with leading zeroes.
Pads address values with leading zeroes to 32-bit.
Pads address values with leading zeroes to 64-bit.
zeroes could also be replaced by 0s. Use should be also aligned with below description text.
@colin-grant-work , @gbodeen , @thegecko , any thoughts about this from native English speakers?
There was a problem hiding this comment.
I think the 'with leading zeroes' part could be a bit redundant, since that's also part of the preference description. Personally, I'd go short, maybe something like
[
"Pads to shortest uniform length", // This one is the trickiest one to describe clearly. Open to suggestions.
"Disables padding",
"Pads to 32 bits",
"Pads to 64 bits"
]There was a problem hiding this comment.
Probably
[
"Pads to largest address width in loaded range", // This one is the trickiest one to describe clearly. Open to suggestions.
"Disables padding",
"Pads to 32 bits",
"Pads to 64 bits"
]
?
|
Tested the changes and the functionality works nicely! |
Introduces VS Code setting for memory address padding with the values * Unpadded * Minimal padding * 32 bit padding * 64 bit padding Fixes eclipse-cdt-cloud#76
martin-fleck-at
force-pushed
the
address-padding
branch
from
00fcb3a to
fe0472f
Compare
|
Since Philip is out this week, I rebased the PR to resolve a merge conflict and improved the wording based on your suggestions. Thank you very much! |
|
Good to merge this @martin-fleck-at ? |
|
Thanks @martin-fleck-at for taking over the maintenance on this PR and thanks @colin-grant-work and @jreineckearm for the reviews. I think this is good to go in and I'll open an issue about the refactoring we were discussing above. Edit: #95 |
What it does
Introduces VS Code setting for memory address padding with the values
Fixes #76
How to test
memory-inspector.addressPaddingin VS Code settingsmemory-address-padding.mp4
Review checklist
Reminder for reviewers