Closed planger closed 5 months ago
Thank you for the fast review @colin-grant-work! I accepted your suggestion and fixed the link. Happy to leave this open until #108 is merged to not promise something before it is actually merged :-)
Also, I'd love to get @jreineckearm's feedback anyway.
@jreineckearm Thank you for your feedback! I think I addressed all your comments and accepted all suggestions.
Thanks for the updates! Looks good to me!
Thanks everyone for their review and suggestions! @colin-grant-work Can you please take another look and, if you're fine with this change, give your approval? Thank you! I'll wait for https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/pull/108 to be merged before I merge this change.
@planger , @colin-grant-work , as discussed today we should add a terminology section and a screenshot to highlight some of the terminology usage. See https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/issues/100#issuecomment-2008003753 for a summary of that call.
I'd recommend to use this PR for those additions. I am happy to draft something and add it here. But it's getting late now. I'll try to provide a write up tomorrow.
Apologies for taking longer than expected. The next comment is a proposal for the wording between "Configuration" and "Contributing". I'll prepare a screenshot next.
The Memory Format settings allow to configure how data that is read from the target system is interpreted and displayed. Use the following to retarget the view to your needs and the inspected memory architecture:
1
for byte-addressable architectures.2
to form a 4-byte value consisting of 2
2-byte MAUs.Autofit
to let the Memory Inspector calculate the best utilization of space in the Data
column.Little Endian
or Big Endian
.The following terminology is used:
Realized now that we should wait with a screenshot until we have the terminology change PR in.
Thanks @jreineckearm! I've added your section to the readme and also updated the screenshot to how it'll likely look like after renaming words to MAUs.
Thanks @planger , may need to do another review iteration of the text and formatting. Already spotted at least one typo in the text I provided. :-) The link to the screenshot seems to be broken on the branch. Gives me a 404 when I click it.
The link to the screenshot seems to be broken on the branch. Gives me a 404 when I click it.
Yes, this link only works after we've merged this change as the image then should become available under the given URL. I preferred to use absolute URLs over relative ones, as this should be safer than something relative when showing the screenshot in the readme of the published extension on the VSX registries.
Edit: but the link actually was wrongly pointing to master
even though the branch is main
in this repo. Fixed with 2f8d50b.
@planger , could you please let me know if attached (modified) screenshot works as a visual reference for the various options? Unfortunately, a little harder to explain endianess in a single one and wouldn't want to put too many screenshots into one sections.
@jreineckearm I like it! Thank you! Just two suggestions for minor tweaks:
Thanks for the feedback, @planger ! Agree with both items. Hope those two images work better. :-)
And another one with the item number. But probably a bit too much.
@jreineckearm Thank you for the images! Imho they look great. I've now used the one with the number regarding endianness. Imho it is more consistent, but I don't have a strong opinion. I cropped the first image a bit to better align both screenshots and avoid a light border.
As you can't preview it on Github because of the absolute link, I put a screenshot below:
Please let me know what you think!
I cropped the first image a bit to better align both screenshots and avoid a light border.
Thanks! I must have missed that border. IMO good to go with that.
Thanks @jreineckearm!
My approval stands - merge at your convenience.
What it does
This change refocuses the README to better resonate with potential users by presenting the extension's features and usage more clearly. We aim to maintain essential information for contributors, ensuring a balance between user orientation and developer guidance.
Also we update the CDT Cloud logo, fixed broken links, and adapted wrong information in the CONTRIBUTING guide left over from Theia.
How to test
Note that the link to the screenshot only works after this is merged. Otherwise we would need to use a relative link, which may not work when displayed in the marketplace or on open-vsx.org.
Review checklist
Reminder for reviewers